Hacker News new | ask | show | jobs
by DoubleFree 1666 days ago
This reminds me of a blog post from Aleksey Kladov: https://matklad.github.io/2021/01/03/two-kinds-of-code-revie...

They state that reviews are not only meant to ensure good code, but also good coders. That means mentoring, but also merging the code if it's not perfect. Then, afterwards, when touching the affected part of the code again, cc the original author in a fixup PR. I have first-hand experience with this way of reviewing and I really like it.
2 comments
nixpulvis 1666 days ago
Why not simply have the discussion in the review itself? Must we merge everything, just so people feel empowered? It's chaos to have to constantly revert and/or refactor everything. People should be empowered not by the merge, but by the progress.

They key, from what I've seen, is to facilitate useful conversation. Keep comments relevant and people will learn. Avoid politicking, while still providing evidence for your case. Typically, show your own work. The best review points clearly towards a better result.

Finally, acknowledge your own limitations as a reviewer. We're all human, to pretend otherwise does yourself and others a massive disservice.

(But what do I know. I've been out of the game for a while now.)

link
bluesnowmonkey 1666 days ago
> Must we merge everything, just so people feel empowered?

Sort of, yes. People develop confidence and expertise from repeated success at accomplishing things. Success creates confidence creates more success. You have to give people room to get that flywheel spinning for themselves.

link
indymike 1666 days ago
> People develop confidence and expertise from repeated success at accomplishing things.

This works just as well, and has the benefit of not introducing bugs and security problems: "Thanks! I can merge this if you fix A and B."

BTW - much better to get the flywheel turning in the right direction early. It is very hard to turn a gyro once it starts spinning.

link
nixpulvis 1666 days ago
I can tell you from recent experience, merging something only to have it reverted later is a net confidence loss.
link
zozbot234 1666 days ago
If it's "not perfect" but still logically correct and useful to the project, then by all means feel free to merge and refactor it later. But broken code that will introduce bugs/defects should not be merged.
link