> - 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.)
"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".
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)
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.
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.
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 <_<
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.
Also, sometimes the answer won't be the one you'd expected.
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?
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.
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 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)
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.
Though specifically for "praise:", I'm a much bigger fan of the :+1: thumbs up emoji instead which accomplishes about the same thing.
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.
Just put some thought and care into the comments.
...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.)
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.
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.
* 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.
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.
I always assume my comments on others' PRs (and other people's comments on my Code Reviews) are "suggestions (non-blocking)", unless indicated otherwise. Shouldn't that be the default, really?
Even though I also often use Github and Gerrit at my current job, everywhere we observe that convention that a comment is considered blocking unless indicated otherwise. This is because ~80% are typically blocking.
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 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.
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...
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 think but I'm not sure that's what js8 meant too.
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.
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."
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.
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.
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!
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.
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).
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.
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...