Conventional Comments
conventionalcomments.orgIt 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?
> - 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.)
It's very easy to seem rude in text comments, even if it was not meant that way at all.
"Change 'this' to 'that'" is much harsher than "Could you change 'this' to 'that'?"
It's a small change in wording, but it changes the interpretation from "This is wrong, do this instead, I know better than you" to "I think this is a better way to do it".
I feel like if this is an issue, there's a larger issue in the team. I've only ever witnessed language be an important issue if people felt overwhelmed, stressed and treated badly. On the other hand, when people feel treated fairly and are happy, most don't care for niceties and prefer straight to the point communication.
Especially rewording change requests as questions is something I don't like. Is this something that should be done? Is it just a question about the technical possibility? A request for comments about the intended change? (I've seen them all and the more friendly they are worded, the harder they are to tell apart)
Is it really?
I can find instructions disguised as questions condescending, for example.
I believe no amount of wording will help if people are not trusting each other. I would rather work on team building than comment guidelines.
But instructions are given by superiors, while code reviews (generally) by peers. If someone you don't respect starts barking "Change 'this' to 'that'", would you?
I mean sure, in some cases you can completely disassociate from the human side of code, disregard a comment's name in favor of objective truth or however you want to word it, but in a lot of instances you still have to share the same office with that person.
Yes and no. It depends on the team and the culture. My favorite was when I worked in a team of only senior engineers we would leave comments like 's/this/that/g other than than LGTM'. This was all that was needed, no fluff.
In other teams, a comment like this would have meant a chat with HR. We can't make rules for these things, because people and thus teams are different.
Whatever you may think, people do see code reviews as conversations, and do take comments personally deep inside, even if they try hard to avoid this in the name of professionalism.
Rather than forcing people to exercise self-discipline while reading, it's nicer to exercise it yourself, and write comments in an impersonal way. Assuming that the reader is as smart as yourself just follows logically from the idea of peer review.
This is what I got from about 10 years of doing code reviews, which sometimes needed to be scathing by nature; to induce the desirable changes you've got to genuinely sound like you're acting in the author's best interests, not just do so.
I haven't done code reviews in forever, but when I did them, sure there were some things that were plain wrong, but there were also a lot of things where I wasn't sure if it actually needed a change - a confirmation from the developer would be required.
And then there's things that I would like to see changed, but as a personal preference; when it comes down to it, it doesn't make a difference and I don't really care, so I would posit it as a suggestion instead of a command. Mind you, if I didn't really care either way, I could've chosen to ignore it as well. But you know how some people get <_<
Yeah, I can agree with "this depends on the kind of work culture etc.".
Asking also seems dishonest if I'm not truly inviting a discussion.
I think it's really worth emphasising this. Further: If the reviewee understands that feedback can be discussed, then the reviewee can discuss whether they're asked or told. If the reviewer has more authority than reviewee, then asking really ought to be asking a question.
...then everybody is stepping on egg shells.
I think the intention behind rephrasing from "this is wrong" to "can it be this instead?" is to try and take the ego out of it. Except people are complicated and 'taking the ego out of it' isn't just down to a question of phrasing.
The reason code reviews are done is to try to catch mistakes earlier. (Mistakes are more expensive to once more development has been done on a task). I like the term "psychological safety". It strikes me as 'unsafe' if a review comments are a matter of the reviewer's ego over the reviewee's. It should be fine to make mistakes, and to have those mistakes corrected. Because people are complicated and emotional.
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.
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.
Yes, but so does just stating something. If it's incorrect, someone will tell you. That's how this forum works, hence your answer ;)
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!
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.
> 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...
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.
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.
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."
Oddly, this has made me want to try Mass Effect.
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.
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?
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.
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.
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.
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.
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:
Feel free to steal it - we've found it works pretty well!
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.
Nerds get a big rush from creating rule systems. And doubly so if then able to foist them on others!
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
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?
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.
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.
I think "passive-aggressive" is a bit too strong. If you leave a comment, even if not blocking, resolving that discussing can simply be a signal that the author of the MR has acknowledged it.
The proposal of conventional comments simply clarifies intention and disambiguates it. It's like that salt and pepper question. How do you know which is in which? Regardless of your knowledge and assumptions, it depends on whoever filled them in. This clarifies that.
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.
GitHub doesn't.
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).
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."
[I agree with you, with qualification]: sometimes we do need to disambiguate by announcing our mental state at the beginning of a comment!
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.
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?
I'm also not a fan of this phrase, which is whipped out to be "helpful" in reviews at my current employer--probably after the last time this website or one similar to it bubbled up on HN.
To be clear, the (American) dictionary definition of nit-picking is "minute and usually unjustified criticism." In other words, this is definitely not a positive thing to do, which is why this word has such strong negative connotations. Who wants to be nitpicked?
Nit-picking is not a "necessary" change, which is what makes it an exercise in finding the tiniest of fungus flies lurking in a proposed change. "I don't really want to think too hard about what you're proposing, or what your pull request contains, so here's a nit I noticed while examining the bike shed in front of your nuclear plant. #didmyjobboss"
I don't know what a trivial but necessary change is, but that's certainly not a minute criticism.
Au contraire: if I give you ten "nitpicks", the fact that it's "minute criticism" means that you explicitly have licence not to feel bad about it. I use the word "nitpick" in a review to induce the reaction of "sigh, I'll make your tiny change", possibly with "I-the-author don't care but apparently you-the-reviewer do for some reason" and "it's all Smaug123's fault for being unreasonable" if the author needs someone to blame mentally for the fact that the PR isn't approved immediately. It's an explicit label/reassurance that no important fault lies with the author.
This is very possibly a team culture thing, though. We all know that the code is what matters, and we are all studiously not letting our egos intervene, but we can help each other not get too attached by deflecting the blame onto the reviewer rather than the author in this tiny way.
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.
"cosmetic"?
Thesaurus gave me carp and quibble. I am going to use carp from now on.
"minor"?
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)
papercut?
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.
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
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.
I agree it's very light, but isn't this what you mean? https://conventionalcomments.org/#format
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.
[taste] It would be cool if this were a blog post or something so I could refer to it someplace more canonical than this post. I would like to try and use it in my own projects.
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.
> 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.)
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.
> 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.
> 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.
Funny. I decided that about coding, perhaps 40 years ago.
But I did later learn enough SQL to crunch data. And I've picked up enough bash to automate stuff.
Most gains in productivity have come from simplifying or automating away common tasks:
* Text editors spell-check your writing (just put more care into your writing!)
* Garbage-collected runtimes,or compile-time memory proofs prevent common memory errors (just write your C more carefully!)
* Machinery and power tools allow one to perform the same physical tasks more quickly (just put your back into it!)
* Agriculture allows one more reliable sources of nutrition & calories (just get good at hunting & gathering!)
Humans have a finite amount of "thought and care" to put into things. If there's a framework that allows for more rapid and reliable software development, with less thought and care spent, that's great. That thought & care can go into something more productive.
>This is just a hack to the problem that most people can’t write
But that is exactly what we need! It will always be easier to change the computers than to change the people. If we could replace all of the people with clones of von Neumann, most development practices (and UX) research would become irrelevant.
Disagree: if most people can't write, then giving them a template that allows them to express themselves efficiently is an excellent idea.
The blogpost needs an introduction: type of comment.
"Introduction" isn't really a tone or a descriptor that changes how you act on the information like the rest.
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.
It changes how I act because it tells me whether to continue reading or not.
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...