4 ms·

I've been pushing our engineers to avoid using GiHub's web UI for PR review. Checkout the branch, open it in the IDE, and walk through the code paths. IDEA has

I've been pushing our engineers to avoid using GiHub's web UI for PR review. Checkout the branch, open it in the IDE, and walk through the code paths. IDEA has a great way to do GitHub PR line comments from the IDE, others likely do too. I've seen more bugs than I like OK'd because the single line change looked innocuous in the web UI review tool. Tools like Pull Panda are worse still, for game-ification of PRs, with leader boards for most PR reviews and fastest reviews. :(

lmilcin 6y ago
Haven't heard about "game-ification" of PRs, but even at first glance this idea sounds horribly misguided. Just as rewarding developers for lines of code written, rewarding them to mark off as many lines of PR as quickly as possible is just so totally moronic I can't begin to comprehend who and for what reason would think that may be a good idea.
- bluGill 6y ago
  When I get a 300 file code review (each file with an average of 100 lines changed) on me I start to wonder if maybe we do need to gamify it just to reward people for breaking it up. Particularity when every single file is making the same set of mistakes (ie every function is prefixed with a 10 line header describing the name, and the types of the parameters - redundant with the name and type info in the one line definition), which makes it harder to find the actual changes of interest.
  - lmilcin 6y ago
    If you get 30k LoC PR you have a problem anyway. Did the author manually modify 30k LoC or is this result of some automated process? If it is for example a refactoring run from IDE (which is completely fine in itself), then the better way to handle it is to separate that single huge change into its own commit that explains how the change was generated. For example, I insist that there are three categories of changes: reformats, refactors and functional changes and that commits can only contain single type of change and that it must be immediately apparent what type of change it is. So if you see 2k LoC reformat you skim over it and don't expect functional changes to be hiding. On the other hand if the code is actual manual modifications it means somebody has spent a huge amount of work on it and it is not unreasonable that Code Review will take proportionally huge amount of work. My longest Code Review took me a week and was for much smaller change in LoC than that. There are probably better ways to handle a change like that. My preferred way is to pair up and assume that if two people actually worked on it, it is as good as a code review.
    - bluGill 6y ago
      I agree with everything until your last paragraph. The people who did this massive thing over several weeks (I exaggerated the size a bit) have a lot of experience, but we don't trust them. They might have paired (though in these pandemic days I'm not sure how well that went), but the pair isn't seen as very good.

lmilcin 6y ago

Haven't heard about "game-ification" of PRs, but even at first glance this idea sounds horribly misguided. Just as rewarding developers for lines of code written, rewarding them to mark off as many lines of PR as quickly as possible is just so totally moronic I can't begin to comprehend who and for what reason would think that may be a good idea.

bluGill 6y ago
When I get a 300 file code review (each file with an average of 100 lines changed) on me I start to wonder if maybe we do need to gamify it just to reward people for breaking it up. Particularity when every single file is making the same set of mistakes (ie every function is prefixed with a 10 line header describing the name, and the types of the parameters - redundant with the name and type info in the one line definition), which makes it harder to find the actual changes of interest.
- lmilcin 6y ago
  If you get 30k LoC PR you have a problem anyway. Did the author manually modify 30k LoC or is this result of some automated process? If it is for example a refactoring run from IDE (which is completely fine in itself), then the better way to handle it is to separate that single huge change into its own commit that explains how the change was generated. For example, I insist that there are three categories of changes: reformats, refactors and functional changes and that commits can only contain single type of change and that it must be immediately apparent what type of change it is. So if you see 2k LoC reformat you skim over it and don't expect functional changes to be hiding. On the other hand if the code is actual manual modifications it means somebody has spent a huge amount of work on it and it is not unreasonable that Code Review will take proportionally huge amount of work. My longest Code Review took me a week and was for much smaller change in LoC than that. There are probably better ways to handle a change like that. My preferred way is to pair up and assume that if two people actually worked on it, it is as good as a code review.
  - bluGill 6y ago
    I agree with everything until your last paragraph. The people who did this massive thing over several weeks (I exaggerated the size a bit) have a lot of experience, but we don't trust them. They might have paired (though in these pandemic days I'm not sure how well that went), but the pair isn't seen as very good.