3 ms·

‘at which point someone complained I was reviewing his PR before it was ready for review (it had WIP in the title)... but I'd always thought that you just don't

by emh68 8y ago

‘at which point someone complained I was reviewing his PR before it was ready for review (it had WIP in the title)... but I'd always thought that you just don't make a PR before you want someone to have a look at it!! Why would you do that?!’ Oh boy. Concrete thinking is a serious issue.

holtalanm 8y ago
submitting a PR then getting annoyed when someone reviews it is also a serious issue. Don't submit a PR until you are ready for eyes on it. End of story. That is literally the entire point of a PR.
- watwut 8y ago
  Meh, respect whatever process is in place now. If team does WIP PR, treat them as WIP. If the team never does WIP PR, keep unfinished work on your machine. If you don't like the process, talk to people. However, don't make damm mess with team process. Slightly odd process (and this is a small thing) that is followed is much better then situation where everyone follows different process. This is level of conformity that is absolutely healthy.
- huehehue 8y ago
  Also want to point out that stacking work on an unreviewed branch isn't a terrific idea, either. I've been asked to review PRs that totally missed the mark, only to find out that there were already a dozen more branches relying on that PR's approval. I'm sure the context is different here, but that's an easy way to set yourself up for redoing weeks of work if you didn't get it right the first time.
- emh68 8y ago
  People definitely misuse PRs though, and while it's common for engineers to go "well that's their problem, I'M logically correct!", I think that's the wrong approach. One very common thing that I've seen is opening a PR just to double-check the way the PR looks on github. Ideally they mark these PRs with "WIP" either in the title or as a tag. It's implied that those PRs aren't ready for review. If you weren't expecting critiques of your code while you were in the middle of developing it (perhaps you have placeholder code all over the place), it could be jarring and distracting. This goes against the purpose of PRs, but at some point you have to put engineering culture flexibility above strict letter-of-the-law guidelines. If someone didn't pick up on this phenomenon, I'd gently inform them. There would be no need for getting upset... unless they continued doing it in spite of the commonly agreed-upon behavior of the team.

by emh68 8y ago

holtalanm 8y ago

submitting a PR then getting annoyed when someone reviews it is also a serious issue. Don't submit a PR until you are ready for eyes on it. End of story. That is literally the entire point of a PR.

watwut 8y ago
Meh, respect whatever process is in place now. If team does WIP PR, treat them as WIP. If the team never does WIP PR, keep unfinished work on your machine. If you don't like the process, talk to people. However, don't make damm mess with team process. Slightly odd process (and this is a small thing) that is followed is much better then situation where everyone follows different process. This is level of conformity that is absolutely healthy.
huehehue 8y ago
Also want to point out that stacking work on an unreviewed branch isn't a terrific idea, either. I've been asked to review PRs that totally missed the mark, only to find out that there were already a dozen more branches relying on that PR's approval. I'm sure the context is different here, but that's an easy way to set yourself up for redoing weeks of work if you didn't get it right the first time.
emh68 8y ago
People definitely misuse PRs though, and while it's common for engineers to go "well that's their problem, I'M logically correct!", I think that's the wrong approach. One very common thing that I've seen is opening a PR just to double-check the way the PR looks on github. Ideally they mark these PRs with "WIP" either in the title or as a tag. It's implied that those PRs aren't ready for review. If you weren't expecting critiques of your code while you were in the middle of developing it (perhaps you have placeholder code all over the place), it could be jarring and distracting. This goes against the purpose of PRs, but at some point you have to put engineering culture flexibility above strict letter-of-the-law guidelines. If someone didn't pick up on this phenomenon, I'd gently inform them. There would be no need for getting upset... unless they continued doing it in spite of the commonly agreed-upon behavior of the team.