3 ms·

The problem is not that the time waiting on a code review can't be used in some other way. It's that the constant interruptions seriously hurt straight-line pr

The problem is not that the time waiting on a code review can't be used in some other way. It's that the constant interruptions seriously hurt straight-line productivity. "OK, done with this minor patch. Now, uhhh, while I wait for code review, I guess I'll go review this other thing... And then start a new patch.... Code review is done now? Better context-switch back." I'm not strictly opposed to pre-commit reviews, but I think it's incorrect to be dismissive of their cost.

mkozlows 11y ago
I agree with this to some extent, but it can be ameliorated a lot with smaller and more frequent PRs. If your PR is enormous and represents a week's worth of work and you end up having to respond to 20 comments, it's going to be awful. If your PR is small and represents hours' worth of work and you have to respond to 1-2 comments, it's a lot less obnoxious. (And you'll have more natural opportunities for context-switching, so responding to comments won't be so distracting.)
- deepGem 11y ago
  I can't agree more to this approach. Smaller changes are easy to review and manaage. We developers need a push to make this happen though.
  - MaulingMonkey 11y ago
    This also needs proper VCS support - by which I mean DVCSes that let you pile commits up to the same files one after another sanely, or reviews after commit rather than before (better have a good branching setup!) My last job was perforce (no unsubmitted commit stacking) with everyone developing directly against a single branch. In theory, code reviews were supposed to happen before hand. In practice, I submitted small changes from the hip, and complained when my coworkers gave me a week long spitball of unrelated features in a single mega-commit. Not that I wasn't guilty of that myself. Bleh.
- yummyfajitas 11y ago
  This works in relatively shallow codebases (e.g. most python/django webapps). In a codebase with deeper dependencies, not so much. - def f(x: X): Option[X] + def f(x: X): MyErrorType \/ X This is not a small change, most users of f now need to be changed as well.
  - couchand 11y ago
    Right, but if that's the only change made, reviewing find-and-replace is trivial. The diff might bleed a bit on the pattern matching, but as long as you restrict the commit to the single logical change, verification is still pretty easy.
    - yummyfajitas 11y ago
      The entire point of such a commit is to not be just a search and replace, but instead to have different behavior depending on the value of the error type. E.g. crash on network error, UPDATE rather than INSERT on specific SQL error. Some changes simply cannot be broken into tiny atomic pieces. Often these are important changes for long term maintainability.
      - couchand 11y ago
        Here's my experience with this sort of change: You have exactly one usage where you NEED the update. After deciding to make it you've identified a few other places where you can use the updated code. The Right Way (TM) to make that change, then, is to make only the change to the result type in the first commit (along with the pattern matching necessary to ignore it), followed by a commit for each place that the updated result type can be usefully used. Change the code without changing the behavior: i.e. REFACTORING. Run the tests, verify you didn't break anything, then start writing tests for the behavior you wish to change, followed by changing that behavior. If you think you're at atoms you probably haven't seen strings.
        yummyfajitas 11y ago
        I suppose it could be done that way. But that seems like a lot of extra work, and you also lose the compilers help. Also, in my experience changes like this actually do involve lots of subtly broken code - think about it, anyone using fold on a network error is making a mistake (retrying the use of a closed connection). If you fix the behavior everyplace the compiler complains, you are guaranteed to resolve most issues (ignoring overloaded methods, e.g. getOrElse). If you use a toOption method on the union type to make a tiny atomic change, the compiler will no longer complain in all the places that mistakes are being made. I suppose an intermediate way might be to rename f to f_DEPRECATED, replace everywhere, and build the new method returning the right type. But in my experience its better just to bite the bullet and change everything.
specialk 11y ago
My team added something four weeks ago that might be an idea to try - we're still trailing it ourselves. It started accidentally and became a tradition on our team though so your mileage may vary. If the pull request is small, fixes a bug or is a blocking you, then you can spend you quick-code-review-chip for the week and ask the team to look at it at their first available opportunity. (Sometimes reviewed within 15 minutes, usually an hour tops) It allows team members to ask the rest of the team to unblock them quickly. Limiting it to one a week prevents abuse of their team mates' time and encourages all team members to plan their work out ahead as to reduce blocking. We also wrote a quick HipChat bot that keeps track of who's spent their quick-review-chip too.
obstinate 11y ago
My team handles this by committing to personal dev branches, then reviewing relatively small merges to mainline. If a review is in progress and you want to work on changes that depend on the code in review, you make another dev branch from the review commit. Then you merge any changes back in after the review is done. Ordinarily this final merge is fairly painless, since you ideally won't have many code reviews that call for sweeping changes in your design. If you do, that's a sign of failure earlier in the process.

mkozlows 11y ago

I agree with this to some extent, but it can be ameliorated a lot with smaller and more frequent PRs. If your PR is enormous and represents a week's worth of work and you end up having to respond to 20 comments, it's going to be awful. If your PR is small and represents hours' worth of work and you have to respond to 1-2 comments, it's a lot less obnoxious. (And you'll have more natural opportunities for context-switching, so responding to comments won't be so distracting.)

deepGem 11y ago
I can't agree more to this approach. Smaller changes are easy to review and manaage. We developers need a push to make this happen though.
- MaulingMonkey 11y ago
  This also needs proper VCS support - by which I mean DVCSes that let you pile commits up to the same files one after another sanely, or reviews after commit rather than before (better have a good branching setup!) My last job was perforce (no unsubmitted commit stacking) with everyone developing directly against a single branch. In theory, code reviews were supposed to happen before hand. In practice, I submitted small changes from the hip, and complained when my coworkers gave me a week long spitball of unrelated features in a single mega-commit. Not that I wasn't guilty of that myself. Bleh.
yummyfajitas 11y ago
This works in relatively shallow codebases (e.g. most python/django webapps). In a codebase with deeper dependencies, not so much. - def f(x: X): Option[X] + def f(x: X): MyErrorType \/ X This is not a small change, most users of f now need to be changed as well.
- couchand 11y ago
  Right, but if that's the only change made, reviewing find-and-replace is trivial. The diff might bleed a bit on the pattern matching, but as long as you restrict the commit to the single logical change, verification is still pretty easy.
  - yummyfajitas 11y ago
    The entire point of such a commit is to not be just a search and replace, but instead to have different behavior depending on the value of the error type. E.g. crash on network error, UPDATE rather than INSERT on specific SQL error. Some changes simply cannot be broken into tiny atomic pieces. Often these are important changes for long term maintainability.
    - couchand 11y ago
      Here's my experience with this sort of change: You have exactly one usage where you NEED the update. After deciding to make it you've identified a few other places where you can use the updated code. The Right Way (TM) to make that change, then, is to make only the change to the result type in the first commit (along with the pattern matching necessary to ignore it), followed by a commit for each place that the updated result type can be usefully used. Change the code without changing the behavior: i.e. REFACTORING. Run the tests, verify you didn't break anything, then start writing tests for the behavior you wish to change, followed by changing that behavior. If you think you're at atoms you probably haven't seen strings.
      - yummyfajitas 11y ago
        I suppose it could be done that way. But that seems like a lot of extra work, and you also lose the compilers help. Also, in my experience changes like this actually do involve lots of subtly broken code - think about it, anyone using fold on a network error is making a mistake (retrying the use of a closed connection). If you fix the behavior everyplace the compiler complains, you are guaranteed to resolve most issues (ignoring overloaded methods, e.g. getOrElse). If you use a toOption method on the union type to make a tiny atomic change, the compiler will no longer complain in all the places that mistakes are being made. I suppose an intermediate way might be to rename f to f_DEPRECATED, replace everywhere, and build the new method returning the right type. But in my experience its better just to bite the bullet and change everything.

specialk 11y ago

My team added something four weeks ago that might be an idea to try - we're still trailing it ourselves. It started accidentally and became a tradition on our team though so your mileage may vary. If the pull request is small, fixes a bug or is a blocking you, then you can spend you quick-code-review-chip for the week and ask the team to look at it at their first available opportunity. (Sometimes reviewed within 15 minutes, usually an hour tops) It allows team members to ask the rest of the team to unblock them quickly. Limiting it to one a week prevents abuse of their team mates' time and encourages all team members to plan their work out ahead as to reduce blocking. We also wrote a quick HipChat bot that keeps track of who's spent their quick-review-chip too.

obstinate 11y ago

My team handles this by committing to personal dev branches, then reviewing relatively small merges to mainline. If a review is in progress and you want to work on changes that depend on the code in review, you make another dev branch from the review commit. Then you merge any changes back in after the review is done. Ordinarily this final merge is fairly painless, since you ideally won't have many code reviews that call for sweeping changes in your design. If you do, that's a sign of failure earlier in the process.