
Conventional Comments - souldzin
https://conventionalcomments.org/
======
hcarvalhoalves
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.)

~~~
zerd
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".

~~~
hcarvalhoalves
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.

~~~
Cthulhu_
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.

------
souldzin
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!

------
jchw
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.

~~~
souldzin
> 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...](https://gitlab.com/conventionalcomments/conventionalcomments.gitlab.io/-/merge_requests/12)

------
GGfpc
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

~~~
bitwize
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
Oddly, this has made me want to try Mass Effect.

------
munk-a
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
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
Feel free to steal it - we've found it works pretty well!

------
Phlogistique
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.

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

~~~
preordained
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

------
tomp
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?

~~~
kubanczyk
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
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.

~~~
andr3
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.

------
mgkimsal
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).

------
renewiltord
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
[I agree with you, with qualification]: sometimes we do need to disambiguate
by announcing our mental state at the beginning of a comment!

------
aabbccdde
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?

~~~
Eiriksmal
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.

~~~
Smaug123
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.

------
intrepidhero
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.

------
sradman
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
I agree it's very light, but isn't this what you mean?
[https://conventionalcomments.org/#format](https://conventionalcomments.org/#format)

------
woile
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

------
zestyping
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.

~~~
aboodman
[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.

------
jevgeni
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
> 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
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
> 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.

------
js8
The blogpost needs an introduction: type of comment.

~~~
AgentME
"Introduction" isn't really a tone or a descriptor that changes how you act on
the information like the rest.

~~~
roelschroeven
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.

------
themodelplumber
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...

