9 ms·

Conventional Comments

156 points by souldzin 6y ago

souldzin 6y ago
Very much inspired from Conventional Commits, this expands the idea to review comments. At GitLab, we are a fully remote organization so we do a lot of written communication. This has been a great pattern for improving readability and the content of review comments. Plus, it's machine parseable which means we could easily query and aggregate these comments in the future!
GGfpc 6y ago
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
- deleted 6y ago
  [deleted]
- bitwize 6y ago
  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."
  - vageli 6y ago
    Oddly, this has made me want to try Mass Effect.
- darkerside 6y ago
  Yeah, those in particular fall flat. If you want to praise, you should do so, and it should be clear in your statement. If it needs the descriptor, the descriptor won't help. I'd suggest s/praise/note. And s/chore/request. Just because it sounds more polite.
- leipert 6y ago
  Disclaimer: I am a coworker of the author. 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.
  - JimDabell 6y ago
    I would strongly dislike being a recipient of this "praise". The thought of all of my co-workers following this rule when reviewing my merge requests makes my skin crawl. You can see from the rest of the thread that other people have a negative reaction to it too. It's nice that you enjoy it but the same does not hold true for everybody. Is there a way for somebody working with you opting out from it or are they given no choice in the matter?
    - joombaga 6y ago
      It's communication coming from other humans. I presume you could just tell them you would strongly dislike being a recipient of this "praise" and update documentation accordingly.
- hkai 6y ago
  Isn't that great to sound robotic? The issue is that many people seem to take code review comments extremely personally and get very upset and defensive. Meanwhile, we rarely get upset at automated eslint warnings, compiler errors or issues found by Google PageSpeed Insights. Sounding robotic prevents you from sounding offensive.
- AgentME 6y ago
  I think those two examples are a bit over-the-top, but I wonder if there might be a benefit to having such a visible convention, partly just because it's such an obvious reminder to be explicit about tone. Seeing it communicates tone and communicates that the reader should also go through extra lengths to communicate their own tone. Though specifically for "praise:", I'm a much bigger fan of the :+1: thumbs up emoji instead which accomplishes about the same thing.
Phlogistique 6y ago
This sounds great! I have often felt the lack of a common language to specify for each review comment if an action is expected. This looks like this convention solves the problem with a nice Schelling point. nitpick (non-blocking): I won't use the "praise:" keyword as it feels robotic. Praise should feel spontaneous and authentic. But I love the general advice of trying to have at least one praise comment per code review.
aabbccdde 6y ago
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?
- memorythought 6y ago
  papercut?
  - deleted 6y ago
    [deleted]
- naniwaduni 6y ago
  "minor"?
- munk-a 6y ago
  I wouldn't say nitpicks are necessary changes in most circumstances - they are really soft suggestions about very fine details, I tend to describe them with optional non-blocking comments. Nitpick is a really good term for them but "aesthetic" might work as an alternative, there isn't a huge glut of words in that realm though.
  - roelschroeven 6y ago
    "cosmetic"?
- leipert 6y ago
  Thesaurus gave me carp and quibble. I am going to use carp from now on.
- opportune 6y ago
  off-topic:, suggestion:, consider: I use nit, but usually for not necessary changes. If my only reviews on a change are nits, I leave the comments but give the greenlight for merging the change (at which point the author may ignore the nits or address them at their discretion)
intrepidhero 6y ago
I like these ideas a lot and if I was mentoring someone in leaving feedback I would strongly encourage providing examples along with suggestions. I like that the "suggestion" label provides a little mental prompt to do that. The easier it is to take action on feedback the better. A diff patch or PR would be ideal. In the office environment where I find I'm often using MS Word with track changes to collaborate (horrors!) I'd much rather a reviewer make a tracked change and leave a comment in the document then send me an email. The flip side is reducing friction for reviewers by making your document easily accessible, and easy to change and comment on, pays dividends.
jevgeni 6y ago
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.
- derefr 6y ago
  > most people can’t write and don’t know how to write something that’s valuable to the reader ...yes? And? 99.999% of people's comparative advantage is not in their writing ability. Add to this that learning to write to a quality level required for effective, efficient communication, is a far higher burden to place on someone, than the level of fluency required for basic exchange of information; especially for people for whom English (or whatever language is being used) is a second language. Taken together, this implies that for most people, learning to write well—i.e. learning the skill that, having possessed it, would make you into a competent essayist—would be a waste of time. Just like learning abstract mathematics would be a waste of time. (And I say that as a professional author!) Most people intuitively know that learning to write more effectively isn't the best use of their time, and so most people don't try. This "hack" is far less costly, and therefore far more likely to get adopted by these people, and therefore far more likely to actually improve the experience of reading the average communication in a large software project (which tends to involve people of varying communication levels.)
  - MAGZine 6y ago
    I abhor the idea that learning to communicate effectively is "a waste of time." Rather than try to avoid learning an incredibly useful skill, people who CAN'T communicate effectively could use code review as a learning/skill building opportunity. If I switch "communicate," with "program," do you still think it's a good argument that people shouldn't obtain any more programming skill than the most rudimentary level, because it would be a waste of time? Conversely, I think clear communication will probably help you write better, clearer, more concise code. It's a win-win.
    - derefr 6y ago
      > Do you still think it's a good argument that people shouldn't obtain any more programming skill than the most rudimentary level, because it would be a waste of time? ...if programming isn't their comparative advantage, then yes. A project manager, or a salesman, should not spend their time learning to code. It's not the most valuable thing they could be doing to increase their ability to do their own job. > I think clear communication will probably help you write better, clearer, more concise code. You realize that communication occurs in a specific language, and the problem is often more to do with lack of knowledge of the language the communication is occurring in, right? Someone might be extremely eloquent in e.g. Spanish, but that doesn't mean they're going to be able to communicate effectively in English. But that also doesn't mean that they aren't going to be able to write good, clear, concise code. They already know how to communicate well; they already have the meta-skill that results in both good writing and good code. They just don't have the specific skill of arranging powerful, pithy words of English to concisely represent a complex concept.
munk-a 6y ago
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.
- leipert 6y ago
  Uh I like the idea of RFC keywords. For reference the RFC2119 which defines them: https://tools.ietf.org/html/rfc2119 https://tools.ietf.org/html/rfc2119
  - munk-a 6y ago
    Feel free to steal it - we've found it works pretty well!
woile 6y ago
I've started doing something like this naturally. I felt the need to communicate it could be improved but the code per se is not wrong. I started using suggestion, optional, minor, etc. This seems like a formalization of what I had in mind, thanks
torinrittenberg 6y ago
this is a conventional comment
tomp 6y ago
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?
- H8crilA 6y ago
  Don't the tools you use allow for selecting whether a comment is blocking or not? This is like the #3 feature of code review software. #2 would be the "approve" button, and #1 would be showing the diff.
  - joombaga 6y ago
    GitHub doesn't.
- kubanczyk 6y ago
  If you use gitlab for code reviews, it's actually the opposite. Each comment thread prevents a merge unless you hit the "Resolve thread" button. It's configurable, but AFAIR it's Gitlab's default. 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.
  - tomp 6y ago
    Interesting, so "passive-agressive" is already built-in. Meaning even commenting in a very innocuous style "I'm not sure if this is correct according to the style guide..." would prevent a merge. I personally try to encourage both communication and responsibility. So I comment on pretty much everything I don't understand/like/agree with (i.e. over-communication, that's really the only way to ensure adequate communication because by over-communicating according to your standards you're more likely hitting the other person's communication standards, which are necessarily different from your own and fundamentally unknowable), but at the same time I encourage the other person to decide which comments they want to respond to (either by arguing or by changing the code), hence showing I trust them and encouraging their personal responsibility.
jchw 6y ago
I agree with the vast majority of this, but I have a bad reaction to “praise” and the idea that you should try to have one per review. I hate praise that feels forced, and it seems like this kind of practice would make it seem quite forced. 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.
- nine_k 6y ago
  Indeed! The praise should be genuine (the author finally fixes a bothersome issue, comes up with a particularly nice solution, etc), and compact. Often a :thumb-up: is totally sufficient.
- souldzin 6y ago
  > I hate praise that feels forced, and it seems like this kind of practice would make it seem quite forced. This is a really good point. Yeah, the `praise` comment is a call to find something to sincerely praise. If you can't find anything to praise (which is probably a warning sign that your head isn't in a happy place) don't leave a `praise`. I updated the description with a little warning about this. What do you think? https://gitlab.com/conventionalcomments/conventionalcomments.gitlab.io/-/merge_requests/12 https://gitlab.com/conventionalcomments/conventionalcomments...
- battery_cowboy 6y ago
  Someone's it's good just to butter someone up with an obvious but true bit of praise. It makes them feel better about themselves and they also like you more as a person because you said something nice. For example, if I told my coworker in a meeting that I was impressed by how smooth her deployment went that morning, even though we both know it wasn't hard to get it right because there wasn't much new, it still makes her feel better about her role at the company and how professional she is, clearly motivational, and maybe later she is more inclined to help me out when I've hit a wall on my project, which makes us both more efficient. It also helps her to know I'm not a jerk, so later if I give her criticism she knows I'm trying to help her because I'm not always just the guy who criticizes, I also say positive remarks. Clearly it has to be genuine praise, but I'm sure everyone can find one thing to praise in any PR.
hcarvalhoalves 6y ago
It seems I disagree w/ many points. > - Leave actionable comments > suggestion: This is not worded correctly. > Can we change this to match the wording of the marketing page? If I already know how it should be, I prefer to quote it right away and save work, instead of asking and leaving room for misinterpretation. Asking also seems dishonest if I'm not truly inviting a discussion. I would still add a rationale so others can correct me though ("Changing this to match the wording on the marketing page"). > - Combine similar comments Sometimes it's better to be repetitive in the name of exactness, because most review tools are line-oriented. Otherwise I have to write a comment "See lines A, B, C, D, E", and the person asking for the review now has more work to do searching all the instances. > - Replace “you” with “we” Better yet, I replace all pronouns w/ "this". Criticise the work, don't make it personal if it's not necessary. "We" feels forced team-building, and dishonest if I'm not actually going to put in work. 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.)
- brailsafe 6y ago
  Agreed. My thought was at least that the tone of the chosen rhetoric seemed sort of passive-aggressive or meek. I tend to prefer use of phrases like "suggestion: This seems like it would be improved by more closely resembling the other marketing pages". That said, asking a question is a communication tactic that is supposed to illicit a response, however disingenuous it may be.
  - nine_k 6y ago
    Asking a question makes the reader find the answer, and thus understand it, and the reasons for it. Also, sometimes the answer won't be the one you'd expected.
    - brailsafe 6y ago
mgkimsal 6y ago
Why the 'we' if 'we' aren't going to do it, but I am? I've had one interesting/productive 6 month engagement with a team where there were some of these comments, but sometimes... someone would actually just make the change in the branch. It saved time, let them get things "the way they wanted" and was generally agreeable to most on the team. Someone making a change in 'my' branch wasn't an affront - it was a degree of collaboration. It was almost always preceded with a message of "hey, I see a small issue here - I'm gonna throw a small patch on it - let me call you after to discuss" or followed up with "hey - fyi - I saw a small bug and did a small patch/test - do you have a few minutes to talk at 3 about it?" And... I would do the same with them, when warranted. It cut down on the delays and let people "take ownership". In the 'wrong hands', I'm sure it could get politically ugly, but no more so than a lot of passive-aggressive comments and blocking/nitpick stuff that holds things up unnecessarily. One thing that was expected (and made it easier) was that whenever you did that (formal or informal review) you were expected to have pulled the code, installed/run the tests locally, run the code, and verified tests ran before you pushed anything back. A note saying "this doesn't seem right" carries different weight from someone giving it a 2 minute glance vs someone who's actually run that code and seen what you've seen. Yes, I know that often you can spot issues without running the code, but... not always (or, if you see the totality of the code in-situ, vs just diffs, you have more context).
themodelplumber 6y ago
Dream: Can we please get this on YouTube. User comments there are a disaster and they're making the entire platform worse. Thank you for making this...and to the naysayers, suggestion: Do it better or stop blocking. This is miles better than doing nothing. And downvote, like always, if it makes you feel better...
zestyping 6y ago
On a previous engineering team, I once proposed a comment tagging convention that evolved over time to a stable place and ended up being quite useful for us. Unlike Conventional Comments, though, our main motivation was to communicate _expectations_ clearly, so the person receiving the review has enough information to decide what _action_ to take. The Conventional Comments labels help a little, but requests for action are lumped under "suggestion" and "nitpick" and "chore", which I don't find to be useful distinctions. "Suggestion" doesn't tell me whether the reviewer is just offering an idea for improvement or pointing out something that must be addressed, and doesn't tell me whether I need to check back with the reviewer before merging my change. If I get a "question", then after I answer the question, is it okay to merge or not? "Issue" doesn't tell me if the issue is big enough to block the PR. —— Here's our system: • [help] means "I need help understanding this PR". I can't do a useful review of this PR without a better understanding. Please talk to me before proceeding. • [fix] means "fix now". I've identified a problem; please fix it before merging. If you don't agree that this needs to be fixed, or you think we should do something different from my proposed action, please discuss it with me before merging -- we probably have a significant difference in understanding that is important to sort out. • [fix?] means "fix now if bug". This looks like it might be a bug, but I'm not sure; please evaluate it, and if it's indeed a bug, treat it as a [fix]. • [minifix] means "fix now if small". I'm asking for this to be fixed before merging because I think it will take <30 seconds. If it takes longer, forget it. • [postfix] means "postponable fix". This is important enough that it must be fixed, but it doesn't need to delay urgent work. You can decide if you want to do it before or after merging this PR. • [cbb] means "optional, objective" ("could be better"). This could be better in the way I'm suggesting, but it's not so important that it must be fixed before merging. I'm making a recommendation as a good practice for next time. • [taste] means "optional, subjective". I'm suggesting an improvement, and I'm acknowledging that it's a personal preference. I am not demanding that you change your preferences long-term, but I'm offering it because I find this practice useful and think you might find it useful too. If you like it, use it. • [fyi] means "for your information". No action needed. This might be mentoring advice, or a reference to something else that's relevant, or a thing to note for the future, etc. —— I like this system because each tag makes it very clear exactly what action is required before merging and under what conditions there needs to be further discussion with the reviewer. [help], [fix], and [fix?] are blocking: the PR cannot be merged as-is without discussion. For [fix] the communication loop must be closed with the reviewer; for [fix?] the communication loop can be open. [minifix], [postfix], [cbb], and [taste] are non-blocking: each one identifies _why_ the suggestion was made so the receiver can decide whether or not to take action. In practice, we found that these 4 categories did pretty well at covering the most common reasons behind suggested actions. Notice: [fix] and [minifix] aren't about whether the change is big or small. A [fix] could be big or small; the important information is that it's mandatory. The smallness indicated by [minifix] is relevant only because the smallness is the _reason_ for the suggestion. Notice: There's no [chore] or [style] or [nitpick] category. [style] doesn't help, because we still don't know whether the style change is mandatory or merely recommended. It doesn't matter what kind of thing we're asking for; what matters is what needs to be done about it.
renewiltord 6y ago
I appreciate the intention behind these things. However, I do not feel like this method is better than to just use the English language, if everyone is fluent in it. For instance, `praise: nice test` sounds a little weirdly robotic in the way that if you were to walk up to someone and say "I am now going to give you a compliment. You are wearing a nice dress."
- Smaug123 6y ago
  [I agree with you, with qualification]: sometimes we do need to disambiguate by announcing our mental state at the beginning of a comment!
- duckmysick 6y ago
  The tagline of the page is "Comments that are easy to grok and grep". If it sounds robotic, it's because a part of the target audience are robots, so to speak. And they are not fluent in English, whether they are using regex and wildcards or statistical sentiment analysis.
js8 6y ago
The blogpost needs an introduction: type of comment.
- AgentME 6y ago
  "Introduction" isn't really a tone or a descriptor that changes how you act on the information like the rest.
  - roelschroeven 6y ago
    My feeling is that the blogpost needs some introduction, stating at least the context of the comments it talks about. Browsing through the comments here at seems about code review, but the blogpost itself never says that. I think but I'm not sure that's what js8 meant too.
  - js8 6y ago
    It changes how I act because it tells me whether to continue reading or not.
frou_dh 6y ago
Nerds get a big rush from creating rule systems. And doubly so if then able to foist them on others!
- preordained 6y ago
  Yah, I have to say material like this getting any attention really accentuates the feeling I get that we're scraping the bottom of the barrel for how to improve in software. It seems like we're more and more focused on the (relatively) superficial...language ergonomics, conventions, etc
sradman 6y ago
This document is inspired by Conventional Commits but lacks the equivalent Specification section. The point of adding structure to unstructured text is specifying the syntax required to make it machine readable. Maybe I’m missing something.
- andr3 6y ago
  I agree it's very light, but isn't this what you mean? https://conventionalcomments.org/#format https://conventionalcomments.org/#format

156 points by souldzin 6y ago

Very much inspired from Conventional Commits, this expands the idea to review comments. At GitLab, we are a fully remote organization so we do a lot of written communication. This has been a great pattern for improving readability and the content of review comments. Plus, it's machine parseable which means we could easily query and aggregate these comments in the future!

GGfpc 6y ago

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

deleted 6y ago
[deleted]
bitwize 6y ago
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."
- vageli 6y ago
  Oddly, this has made me want to try Mass Effect.
darkerside 6y ago
Yeah, those in particular fall flat. If you want to praise, you should do so, and it should be clear in your statement. If it needs the descriptor, the descriptor won't help. I'd suggest s/praise/note. And s/chore/request. Just because it sounds more polite.
leipert 6y ago
Disclaimer: I am a coworker of the author. 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.
- JimDabell 6y ago
  I would strongly dislike being a recipient of this "praise". The thought of all of my co-workers following this rule when reviewing my merge requests makes my skin crawl. You can see from the rest of the thread that other people have a negative reaction to it too. It's nice that you enjoy it but the same does not hold true for everybody. Is there a way for somebody working with you opting out from it or are they given no choice in the matter?
  - joombaga 6y ago
    It's communication coming from other humans. I presume you could just tell them you would strongly dislike being a recipient of this "praise" and update documentation accordingly.
hkai 6y ago
Isn't that great to sound robotic? The issue is that many people seem to take code review comments extremely personally and get very upset and defensive. Meanwhile, we rarely get upset at automated eslint warnings, compiler errors or issues found by Google PageSpeed Insights. Sounding robotic prevents you from sounding offensive.
AgentME 6y ago
I think those two examples are a bit over-the-top, but I wonder if there might be a benefit to having such a visible convention, partly just because it's such an obvious reminder to be explicit about tone. Seeing it communicates tone and communicates that the reader should also go through extra lengths to communicate their own tone. Though specifically for "praise:", I'm a much bigger fan of the :+1: thumbs up emoji instead which accomplishes about the same thing.

Phlogistique 6y ago

This sounds great! I have often felt the lack of a common language to specify for each review comment if an action is expected. This looks like this convention solves the problem with a nice Schelling point. nitpick (non-blocking): I won't use the "praise:" keyword as it feels robotic. Praise should feel spontaneous and authentic. But I love the general advice of trying to have at least one praise comment per code review.

aabbccdde 6y ago

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?

memorythought 6y ago
papercut?
- deleted 6y ago
  [deleted]
naniwaduni 6y ago
"minor"?
munk-a 6y ago
I wouldn't say nitpicks are necessary changes in most circumstances - they are really soft suggestions about very fine details, I tend to describe them with optional non-blocking comments. Nitpick is a really good term for them but "aesthetic" might work as an alternative, there isn't a huge glut of words in that realm though.
- roelschroeven 6y ago
  "cosmetic"?
leipert 6y ago
Thesaurus gave me carp and quibble. I am going to use carp from now on.
opportune 6y ago
off-topic:, suggestion:, consider: I use nit, but usually for not necessary changes. If my only reviews on a change are nits, I leave the comments but give the greenlight for merging the change (at which point the author may ignore the nits or address them at their discretion)

intrepidhero 6y ago

I like these ideas a lot and if I was mentoring someone in leaving feedback I would strongly encourage providing examples along with suggestions. I like that the "suggestion" label provides a little mental prompt to do that. The easier it is to take action on feedback the better. A diff patch or PR would be ideal. In the office environment where I find I'm often using MS Word with track changes to collaborate (horrors!) I'd much rather a reviewer make a tracked change and leave a comment in the document then send me an email. The flip side is reducing friction for reviewers by making your document easily accessible, and easy to change and comment on, pays dividends.

jevgeni 6y ago

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.

derefr 6y ago
> most people can’t write and don’t know how to write something that’s valuable to the reader ...yes? And? 99.999% of people's comparative advantage is not in their writing ability. Add to this that learning to write to a quality level required for effective, efficient communication, is a far higher burden to place on someone, than the level of fluency required for basic exchange of information; especially for people for whom English (or whatever language is being used) is a second language. Taken together, this implies that for most people, learning to write well—i.e. learning the skill that, having possessed it, would make you into a competent essayist—would be a waste of time. Just like learning abstract mathematics would be a waste of time. (And I say that as a professional author!) Most people intuitively know that learning to write more effectively isn't the best use of their time, and so most people don't try. This "hack" is far less costly, and therefore far more likely to get adopted by these people, and therefore far more likely to actually improve the experience of reading the average communication in a large software project (which tends to involve people of varying communication levels.)
- MAGZine 6y ago
  I abhor the idea that learning to communicate effectively is "a waste of time." Rather than try to avoid learning an incredibly useful skill, people who CAN'T communicate effectively could use code review as a learning/skill building opportunity. If I switch "communicate," with "program," do you still think it's a good argument that people shouldn't obtain any more programming skill than the most rudimentary level, because it would be a waste of time? Conversely, I think clear communication will probably help you write better, clearer, more concise code. It's a win-win.
  - derefr 6y ago
    > Do you still think it's a good argument that people shouldn't obtain any more programming skill than the most rudimentary level, because it would be a waste of time? ...if programming isn't their comparative advantage, then yes. A project manager, or a salesman, should not spend their time learning to code. It's not the most valuable thing they could be doing to increase their ability to do their own job. > I think clear communication will probably help you write better, clearer, more concise code. You realize that communication occurs in a specific language, and the problem is often more to do with lack of knowledge of the language the communication is occurring in, right? Someone might be extremely eloquent in e.g. Spanish, but that doesn't mean they're going to be able to communicate effectively in English. But that also doesn't mean that they aren't going to be able to write good, clear, concise code. They already know how to communicate well; they already have the meta-skill that results in both good writing and good code. They just don't have the specific skill of arranging powerful, pithy words of English to concisely represent a complex concept.

munk-a 6y ago

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.

leipert 6y ago
Uh I like the idea of RFC keywords. For reference the RFC2119 which defines them: https://tools.ietf.org/html/rfc2119 https://tools.ietf.org/html/rfc2119
- munk-a 6y ago
  Feel free to steal it - we've found it works pretty well!

woile 6y ago

I've started doing something like this naturally. I felt the need to communicate it could be improved but the code per se is not wrong. I started using suggestion, optional, minor, etc. This seems like a formalization of what I had in mind, thanks

torinrittenberg 6y ago

this is a conventional comment

tomp 6y ago

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?

H8crilA 6y ago
Don't the tools you use allow for selecting whether a comment is blocking or not? This is like the #3 feature of code review software. #2 would be the "approve" button, and #1 would be showing the diff.
- joombaga 6y ago
  GitHub doesn't.
kubanczyk 6y ago
If you use gitlab for code reviews, it's actually the opposite. Each comment thread prevents a merge unless you hit the "Resolve thread" button. It's configurable, but AFAIR it's Gitlab's default. 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.
- tomp 6y ago
  Interesting, so "passive-agressive" is already built-in. Meaning even commenting in a very innocuous style "I'm not sure if this is correct according to the style guide..." would prevent a merge. I personally try to encourage both communication and responsibility. So I comment on pretty much everything I don't understand/like/agree with (i.e. over-communication, that's really the only way to ensure adequate communication because by over-communicating according to your standards you're more likely hitting the other person's communication standards, which are necessarily different from your own and fundamentally unknowable), but at the same time I encourage the other person to decide which comments they want to respond to (either by arguing or by changing the code), hence showing I trust them and encouraging their personal responsibility.

jchw 6y ago

I agree with the vast majority of this, but I have a bad reaction to “praise” and the idea that you should try to have one per review. I hate praise that feels forced, and it seems like this kind of practice would make it seem quite forced. 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.

nine_k 6y ago
Indeed! The praise should be genuine (the author finally fixes a bothersome issue, comes up with a particularly nice solution, etc), and compact. Often a :thumb-up: is totally sufficient.
souldzin 6y ago
> I hate praise that feels forced, and it seems like this kind of practice would make it seem quite forced. This is a really good point. Yeah, the `praise` comment is a call to find something to sincerely praise. If you can't find anything to praise (which is probably a warning sign that your head isn't in a happy place) don't leave a `praise`. I updated the description with a little warning about this. What do you think? https://gitlab.com/conventionalcomments/conventionalcomments.gitlab.io/-/merge_requests/12 https://gitlab.com/conventionalcomments/conventionalcomments...
battery_cowboy 6y ago
Someone's it's good just to butter someone up with an obvious but true bit of praise. It makes them feel better about themselves and they also like you more as a person because you said something nice. For example, if I told my coworker in a meeting that I was impressed by how smooth her deployment went that morning, even though we both know it wasn't hard to get it right because there wasn't much new, it still makes her feel better about her role at the company and how professional she is, clearly motivational, and maybe later she is more inclined to help me out when I've hit a wall on my project, which makes us both more efficient. It also helps her to know I'm not a jerk, so later if I give her criticism she knows I'm trying to help her because I'm not always just the guy who criticizes, I also say positive remarks. Clearly it has to be genuine praise, but I'm sure everyone can find one thing to praise in any PR.

hcarvalhoalves 6y ago

It seems I disagree w/ many points. > - Leave actionable comments > suggestion: This is not worded correctly. > Can we change this to match the wording of the marketing page? If I already know how it should be, I prefer to quote it right away and save work, instead of asking and leaving room for misinterpretation. Asking also seems dishonest if I'm not truly inviting a discussion. I would still add a rationale so others can correct me though ("Changing this to match the wording on the marketing page"). > - Combine similar comments Sometimes it's better to be repetitive in the name of exactness, because most review tools are line-oriented. Otherwise I have to write a comment "See lines A, B, C, D, E", and the person asking for the review now has more work to do searching all the instances. > - Replace “you” with “we” Better yet, I replace all pronouns w/ "this". Criticise the work, don't make it personal if it's not necessary. "We" feels forced team-building, and dishonest if I'm not actually going to put in work. 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.)

brailsafe 6y ago
Agreed. My thought was at least that the tone of the chosen rhetoric seemed sort of passive-aggressive or meek. I tend to prefer use of phrases like "suggestion: This seems like it would be improved by more closely resembling the other marketing pages". That said, asking a question is a communication tactic that is supposed to illicit a response, however disingenuous it may be.
- nine_k 6y ago
  Asking a question makes the reader find the answer, and thus understand it, and the reasons for it. Also, sometimes the answer won't be the one you'd expected.
  - brailsafe 6y ago

mgkimsal 6y ago

Why the 'we' if 'we' aren't going to do it, but I am? I've had one interesting/productive 6 month engagement with a team where there were some of these comments, but sometimes... someone would actually just make the change in the branch. It saved time, let them get things "the way they wanted" and was generally agreeable to most on the team. Someone making a change in 'my' branch wasn't an affront - it was a degree of collaboration. It was almost always preceded with a message of "hey, I see a small issue here - I'm gonna throw a small patch on it - let me call you after to discuss" or followed up with "hey - fyi - I saw a small bug and did a small patch/test - do you have a few minutes to talk at 3 about it?" And... I would do the same with them, when warranted. It cut down on the delays and let people "take ownership". In the 'wrong hands', I'm sure it could get politically ugly, but no more so than a lot of passive-aggressive comments and blocking/nitpick stuff that holds things up unnecessarily. One thing that was expected (and made it easier) was that whenever you did that (formal or informal review) you were expected to have pulled the code, installed/run the tests locally, run the code, and verified tests ran before you pushed anything back. A note saying "this doesn't seem right" carries different weight from someone giving it a 2 minute glance vs someone who's actually run that code and seen what you've seen. Yes, I know that often you can spot issues without running the code, but... not always (or, if you see the totality of the code in-situ, vs just diffs, you have more context).

themodelplumber 6y ago

Dream: Can we please get this on YouTube. User comments there are a disaster and they're making the entire platform worse. Thank you for making this...and to the naysayers, suggestion: Do it better or stop blocking. This is miles better than doing nothing. And downvote, like always, if it makes you feel better...

zestyping 6y ago

On a previous engineering team, I once proposed a comment tagging convention that evolved over time to a stable place and ended up being quite useful for us. Unlike Conventional Comments, though, our main motivation was to communicate _expectations_ clearly, so the person receiving the review has enough information to decide what _action_ to take. The Conventional Comments labels help a little, but requests for action are lumped under "suggestion" and "nitpick" and "chore", which I don't find to be useful distinctions. "Suggestion" doesn't tell me whether the reviewer is just offering an idea for improvement or pointing out something that must be addressed, and doesn't tell me whether I need to check back with the reviewer before merging my change. If I get a "question", then after I answer the question, is it okay to merge or not? "Issue" doesn't tell me if the issue is big enough to block the PR. —— Here's our system: • [help] means "I need help understanding this PR". I can't do a useful review of this PR without a better understanding. Please talk to me before proceeding. • [fix] means "fix now". I've identified a problem; please fix it before merging. If you don't agree that this needs to be fixed, or you think we should do something different from my proposed action, please discuss it with me before merging -- we probably have a significant difference in understanding that is important to sort out. • [fix?] means "fix now if bug". This looks like it might be a bug, but I'm not sure; please evaluate it, and if it's indeed a bug, treat it as a [fix]. • [minifix] means "fix now if small". I'm asking for this to be fixed before merging because I think it will take <30 seconds. If it takes longer, forget it. • [postfix] means "postponable fix". This is important enough that it must be fixed, but it doesn't need to delay urgent work. You can decide if you want to do it before or after merging this PR. • [cbb] means "optional, objective" ("could be better"). This could be better in the way I'm suggesting, but it's not so important that it must be fixed before merging. I'm making a recommendation as a good practice for next time. • [taste] means "optional, subjective". I'm suggesting an improvement, and I'm acknowledging that it's a personal preference. I am not demanding that you change your preferences long-term, but I'm offering it because I find this practice useful and think you might find it useful too. If you like it, use it. • [fyi] means "for your information". No action needed. This might be mentoring advice, or a reference to something else that's relevant, or a thing to note for the future, etc. —— I like this system because each tag makes it very clear exactly what action is required before merging and under what conditions there needs to be further discussion with the reviewer. [help], [fix], and [fix?] are blocking: the PR cannot be merged as-is without discussion. For [fix] the communication loop must be closed with the reviewer; for [fix?] the communication loop can be open. [minifix], [postfix], [cbb], and [taste] are non-blocking: each one identifies _why_ the suggestion was made so the receiver can decide whether or not to take action. In practice, we found that these 4 categories did pretty well at covering the most common reasons behind suggested actions. Notice: [fix] and [minifix] aren't about whether the change is big or small. A [fix] could be big or small; the important information is that it's mandatory. The smallness indicated by [minifix] is relevant only because the smallness is the _reason_ for the suggestion. Notice: There's no [chore] or [style] or [nitpick] category. [style] doesn't help, because we still don't know whether the style change is mandatory or merely recommended. It doesn't matter what kind of thing we're asking for; what matters is what needs to be done about it.

renewiltord 6y ago

I appreciate the intention behind these things. However, I do not feel like this method is better than to just use the English language, if everyone is fluent in it. For instance, `praise: nice test` sounds a little weirdly robotic in the way that if you were to walk up to someone and say "I am now going to give you a compliment. You are wearing a nice dress."

Smaug123 6y ago
[I agree with you, with qualification]: sometimes we do need to disambiguate by announcing our mental state at the beginning of a comment!
duckmysick 6y ago
The tagline of the page is "Comments that are easy to grok and grep". If it sounds robotic, it's because a part of the target audience are robots, so to speak. And they are not fluent in English, whether they are using regex and wildcards or statistical sentiment analysis.

js8 6y ago

The blogpost needs an introduction: type of comment.

AgentME 6y ago
"Introduction" isn't really a tone or a descriptor that changes how you act on the information like the rest.
- roelschroeven 6y ago
  My feeling is that the blogpost needs some introduction, stating at least the context of the comments it talks about. Browsing through the comments here at seems about code review, but the blogpost itself never says that. I think but I'm not sure that's what js8 meant too.
- js8 6y ago
  It changes how I act because it tells me whether to continue reading or not.

frou_dh 6y ago

Nerds get a big rush from creating rule systems. And doubly so if then able to foist them on others!

preordained 6y ago
Yah, I have to say material like this getting any attention really accentuates the feeling I get that we're scraping the bottom of the barrel for how to improve in software. It seems like we're more and more focused on the (relatively) superficial...language ergonomics, conventions, etc

sradman 6y ago

This document is inspired by Conventional Commits but lacks the equivalent Specification section. The point of adding structure to unstructured text is specifying the syntax required to make it machine readable. Maybe I’m missing something.

andr3 6y ago
I agree it's very light, but isn't this what you mean? https://conventionalcomments.org/#format https://conventionalcomments.org/#format