Hacker News Reader

Comment by mike_hearn | Hacker News Reader

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.

View on HN

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.

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.

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.

Yeah, additionally Juniors can read the fix commits and learn from that. Thanks for describing your work flow Mike!

Reply on news.ycombinator.com