3 ms·
I find that I rarely use direct commands in code reviews, even ones expressed indirectly (e.g. "can yo do X" instead of "do X"). If there are blockers in a PR,
by Tainnor 4y ago
I find that I rarely use direct commands in code reviews, even ones expressed indirectly (e.g. "can yo do X" instead of "do X").
If there are blockers in a PR, I'll add reasoning for why it is a problem (e.g. "this exposes us to vulnerability X", "in this edge case, what would happen is that ...", "I tested it and appears that there is a bug", etc.) If it's a team-agreed style issue (that isn't caught by the linter), I'd rather say "we use convention so and so in this code base" than "please change this to this style".
The reasoning is that I feel that I work with professionals and while they may need feedback, they don't really need commands. I can make suggestions as to how they can fix the issue I raised, but ultimately it's up to them to fix it and if they find an even better way that I didn't think of, why should I limit their problem solving capabilities artificially?
I find that most people I've worked with use a similar style (more centered around the code than around power dynamics), and those people who used a lot of "please do X" style comments were usually also pretty defensive about their code or ideas, would get uneasy when you changed some of their code ("why did you refactor this? please change it back") etc.
Now there may be some people (inexperienced, or just idiots) who really need some more forceful language, but you shouldn't necessarily start out on that default assumption.
That covers the blocker-style comments. Everything else should probably be done in either a "I'm opening up a discussion here and would be interested in your opinion" or a "this is just a nitpick, feel free to ignore" kind of style.
---
For the specific example, depending on the reasoning for why you might want the change:
- "The linter is complaining that this function is too long and we typically stick to those rules. I think lines 20-30 could be extracted into a separate function, for example."
- "I found it a bit hard to understand this function initially because it does a number of things at once (such as...). Maybe we could split it up?"
- "This section of code is repeated between here and that other function and I think we should keep the implementations in sync, so I would recommend extracting that common logic."
- "We've actually had to do something similar in other situations (see here and here) and your solution seems better than what we came up with before, so what do you think about extracting this functionality into a function and calling it from all those places?"
and so on (and it doesn't need to be as verbose if you know that you have more shared context, for example).