121–130 of 309 posts
[Suggestion] Use conventional comments[1] to flag how important the comment is. Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in. 1: https://conventionalcomments.org/
Using conventional comments also forces me to rethink if my comment is really blocking or is just an optional suggestion and I can adjust the tone accordingly.
[Suggestion] Use conventional comments[1] to flag how important the comment is. Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in. 1: https://conventionalcomments.org/
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people). These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you a…
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…
I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).
Btw, since I think it is relevant to this discussion, we're working on making code reviews deeper than text based diffs. Check out (DiffLens)[https://github.com/marketplace/difflens] if you're code base is primarily TS, JS and/or CSS to get language aware diffs on your GitHub pull requests. We've found that it makes understanding code changes much easier
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).
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…