21–30 of 74 posts
This is just a hack to the problem that most people can’t write and don’t know how to write something that’s valuable to the reader. Just put some thought and care into the comments.
I'm guessing this is about Code Review comments? I always assume my comments on others' PRs (and other people's comments on my Code Reviews) are "suggestions (non-blocking)", unless indicated otherwise . Shouldn't that be the default, really?
I get the ones where you're suggesting alterations to the code, but having "praise: You did great here" or "chore: Could you run the tests" sounds weirdly robotic to me
I like Praise. We don’t give it often enough and if someone does something awesome like simplifying complicated tests, removing dependencies or works smart and not hard, it is due.
While the conventional comments give a nice framework, nothing stops someone from adding a cheerful gif to the body of that praise.
Even on larger reviews it is nice if you scroll through 10 nitpicks, 20 suggestions and 5 questions, if you see the occasional praise.
Only tangentially relevant: I hate the word 'nitpick' (bad experience with small insects - thinking about nits makes me itch), but I don't know any other word for these "small, trivial, but necessary changes." Does anyone have alternative words they use in reviews instead?
When leaving review comments our internal system is to use a blocking/non-blocking flag along with the classic three RFC keywords (MAY/SHOULD/MUST - often in bold) as two examples: You MAY wish to improve readability here by binding these values to a local variable We're using string building in this query with user input we MUST not allow any SQL injection routes.
When leaving review comments our internal system is to use a blocking/non-blocking flag along with the classic three RFC keywords (MAY/SHOULD/MUST - often in bold) as two examples: You MAY wish to improve readability here by binding these values to a local variable We're using string building in this query with user input we MUST not allow any SQL injection routes.
Uh I like the idea of RFC keywords. For reference the RFC2119 which defines them: https://tools.ietf.org/html/rfc2119
I feel you could just say something like “good idea” and it would be fine without the prefix. Especially if your review tool of choice has a concept of actionable vs non-actionable comments, like Gerrit and resolved/unresolved comments.
I'm guessing this is about Code Review comments? I always assume my comments on others' PRs (and other people's comments on my Code Reviews) are "suggestions (non-blocking)", unless indicated otherwise . Shouldn't that be the default, really?
Even though I also often use Github and Gerrit at my current job, everywhere we observe that convention that a comment is considered blocking unless indicated otherwise. This is because ~80% are typically blocking.
> - Leave actionable comments
> suggestion: This is not worded correctly.
> Can we change this to match the wording of the marketing page?
> - Combine similar comments
> - Replace “you” with “we”
It seems these practices are created because people are using reviews as some form of conversation, and then everybody is stepping on egg shells. I believe if the entire team agrees to see it as just annotations (it's not a performance evaluation, leave your ego at the door, etc) these problems are avoided.
(I can see "why" you need these guidelines though, I guess it depends on what kind of work culture you have, company size, and so on.)
I get the ones where you're suggesting alterations to the code, but having "praise: You did great here" or "chore: Could you run the tests" sounds weirdly robotic to me
It reminds me of the Elcor from Mass Effect. The problem they face is similar to textual comments: Elcor cannot emote in a way that's detectable to non-Elcor, so when communicating with alien races such as humans, they have to prefix every utterance with an emotional descriptor. E.g., "Pleased greeting: Human, it is always good to see your kind."