4 ms·

"it's quicker to just fix the thing you want changed than ask someone else to do it" Sure, but I feel like there's a big disadvantage here in that the person w

"it's quicker to just fix the thing you want changed than ask someone else to do it" Sure, but I feel like there's a big disadvantage here in that the person who did the bad thing isn't actively involved in fixing it. They lose out on acquiring that muscle memory. If this is a Sr. reviewing another Sr.'s PR and it's just a few minor oversights, fine. But if this is a Sr. reviewing a Jr.'s PR and they fix like 10 things in one commit, that seems harmful to the inexperienced dev's development. They don't even get a chance to ask why or provide clarification. And if it comes off like a bunch of nitpicking, that's a person who likely really needs a little handholding. The whole "give a man a fish vs teach" proverb.

mike_hearn 3y ago
You can certainly pick which approach to use based on your wider goals, sure. Sometimes though changes are a matter of style, or something is easier to show rather than tell, or you realize whilst reviewing the requirements weren't quite right (on you) etc. For example, if a junior isn't familiar enough with the standard library and misses a trick, you can try to explain in words what the better way is, or you can just fix it and then ask them to study the diff. I don't know what's better for long term learning, but at least you have the option.
- Corsome 3y ago
  Yeah, additionally Juniors can read the fix commits and learn from that. Thanks for describing your work flow Mike!
- brandall10 3y ago
  You can do a "show don't tell" with a fix suggestion in a Github PR, which one accept as-is or can use for inspiration. I feel like in the majority of cases doing something for someone is a missed opportunity - esp. again in the situation where there are many. A lot of those aren't going to stick for most people if they're simply quickly reviewing it. It also seems like an opportunity for certain personality types to run roughshod over engineers they don't respect.
  - mike_hearn 3y ago
    Yes GitHub is slowly reinventing IDEs in their code review tools. If you do it in the IDE with real commits you can use refactoring aids, search+replace etc. If someone keeps making the same errors and isn't paying attention to the fixes you can just talk about it with them in a meeting, same as with any other recurring problem you might have with a teammate. The model we implement is hierarchical, so there's no running roughshod over other people by design. If the reviewer isn't happy they won't merge, and you have to keep going until they are. But, it's also on the reviewer to get the work in because they're being held accountable for delivery too, and they can make changes with the same tools as the authors. So it's a collaboration.
    - brandall10 3y ago
      What I mean by running roughshod is now it's incumbent on an engineer with lesser power to overwrite changes they don't agree with or understand. The chance of them doing that is going to be low unless the change is obviously wrong. One has to appreciate this power disparity on a team of mixed skill levels. I've been doing this for 26 years at 15 companies and have led several teams. I can think of some where this would be fine (ie. everyone has a healthy amount of EQ), but I can think of several with talented but somewhat thorny, opinionated sr level engineers where this would be a problem. I once had to have a talk with someone on the spectrum who would occasionally do this very thing because it was more expedient than explaining in a PR. The person who complained felt like they were being disrespected.

mike_hearn 3y ago

You can certainly pick which approach to use based on your wider goals, sure. Sometimes though changes are a matter of style, or something is easier to show rather than tell, or you realize whilst reviewing the requirements weren't quite right (on you) etc. For example, if a junior isn't familiar enough with the standard library and misses a trick, you can try to explain in words what the better way is, or you can just fix it and then ask them to study the diff. I don't know what's better for long term learning, but at least you have the option.

Corsome 3y ago
Yeah, additionally Juniors can read the fix commits and learn from that. Thanks for describing your work flow Mike!
brandall10 3y ago
You can do a "show don't tell" with a fix suggestion in a Github PR, which one accept as-is or can use for inspiration. I feel like in the majority of cases doing something for someone is a missed opportunity - esp. again in the situation where there are many. A lot of those aren't going to stick for most people if they're simply quickly reviewing it. It also seems like an opportunity for certain personality types to run roughshod over engineers they don't respect.
- mike_hearn 3y ago
  Yes GitHub is slowly reinventing IDEs in their code review tools. If you do it in the IDE with real commits you can use refactoring aids, search+replace etc. If someone keeps making the same errors and isn't paying attention to the fixes you can just talk about it with them in a meeting, same as with any other recurring problem you might have with a teammate. The model we implement is hierarchical, so there's no running roughshod over other people by design. If the reviewer isn't happy they won't merge, and you have to keep going until they are. But, it's also on the reviewer to get the work in because they're being held accountable for delivery too, and they can make changes with the same tools as the authors. So it's a collaboration.
  - brandall10 3y ago
    What I mean by running roughshod is now it's incumbent on an engineer with lesser power to overwrite changes they don't agree with or understand. The chance of them doing that is going to be low unless the change is obviously wrong. One has to appreciate this power disparity on a team of mixed skill levels. I've been doing this for 26 years at 15 companies and have led several teams. I can think of some where this would be fine (ie. everyone has a healthy amount of EQ), but I can think of several with talented but somewhat thorny, opinionated sr level engineers where this would be a problem. I once had to have a talk with someone on the spectrum who would occasionally do this very thing because it was more expedient than explaining in a PR. The person who complained felt like they were being disrespected.