7 ms·

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylisti

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

znpy 2y ago
on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings). The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.
- prh8 2y ago
  "will approve after nitpicks" is the most asinine behavior
- barbazoo 2y ago
  Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.
- dec0dedab0de 2y ago
  * I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).* I always thought this was the best way. I wish these systems had a way to assign severity to comments, and urgency to the commit. If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.
  - yjftsjthsd-h 2y ago
    Depending on your company culture, you can also just explicitly write something like "This is not a blocker, but I would suggest...". Of course if your culture uses explicit "press the button to unblock" then that's probably redundant.
    - to11mtm 2y ago
      Yeah I do similar and encourage colleagues/orgs to do the same.
    - erik_seaberg 2y ago
      Half of my comments were "nit: consider blah blah" where I want it on his radar, but I approved anyway and if he declines I'm fine with it.
  - fmbb 2y ago
    I like https://blog.danlew.net/2020/04/15/the-logaf-scale/ https://blog.danlew.net/2020/04/15/the-logaf-scale/
- to11mtm 2y ago
  It's tough with spelling sometimes. In my current role I review a lot of PRs, they tend to be large due to the waterfall way things work... If I don't ask to fix the spelling now, It might not happen for a year or two. That said if it can be fixed in separate PR before release, that's fine. Really, it's best to have some terms or explicitness. For example, with my teams 'Nitpick' means I'm just being nitpicky, Doesn't have to change unless it's on the edge (and I'm explicit as to why it should change, i.e. I know the next thing will need the change anyway). "Consider" means It doesn't have to happen, but here's some food for thought. "PLZ FIX" is fairly self explanatory. Also, making sure management (especially for contract houses) knows that PRs are a 'judgement free zone' and should not be held against people for perf reviews etc; that should be collected by other peer feedback channels instead.
- rileymat2 2y ago
  In many dynamic languages, depending on scope, spelling is not a nitpick, it has long term mental costs and very well can cause bugs.
  - vips7L 2y ago
    Spelling automatically should be highlighted by the ide too. Spelling mistakes means you’re blatantly ignoring warnings from the ide.
    - znpy 2y ago
      Please see my other comment. I meant misspelling of English words in the comments. Misspelled code will not compile and/or pass the tests so the pr won’t be accepted anyway.
  - znpy 2y ago
    I should have been less ambiguous: i meant spelling of words on the English language, in comments. Good pr systems will perform a build of the code and run tests, so if the mis-spelling is in the code the build will fail anyway.
  - Aeolun 2y ago
    Sorta, but it’s almost never worth blocking the PR over. Just ask to fix and approve. If it doesn’t happen and you come across it later just fix it.
bluefirebrand 2y ago
Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
- barbazoo 2y ago
  Oh 100%. Discussing style within a PR unrelated to changing the styling rules seems like a waste of time.
- deleted 2y ago
  [deleted]
- DanHulton 2y ago
  This literally what the article says.
- InvaderFizz 2y ago
  I agree. Even within my own org there is some nitpicking, but it's almost always about style consistency for our shared codebase, which is valid. If your PR doesn't pass lint checks, it doesn't get merged. And the only reason it would fail the lint checks is if your pre-commit hooks didn't fire. There is no argument of 2,4,8 space vs tabs, because the code you commit is run through the linter. Write however you want for the things that don't matter, the formatter always wins.
- chuckhend 2y ago
  Automating this with linter and formatter is great. It moves the argument over style and format to a one liner change to a lint config instead of mingling it with the with the main code change.
  - bluefirebrand 2y ago
    It also hopefully only happens once. If you continue to have people bringing up arguments over the linter and formatter after an initial agreement is made, then you can talk to those people
js8 2y ago
I think a good general advice when training somebody is - focus them only on the biggest flaw at the time. That's how I would proceed with a "bad" PR. I would even accepts smaller issues (style) when there is a bigger issue to fix. People might eventually outgrow it.
- eddd-ddde 2y ago
  For me it's the opposite. If I'm making a PR, please point out ALL of the nits you can even think about. My next PR is bound to have at most 25% of that since by now I get more about what the team values and considers important.
  - collingreen 2y ago
    This highlights something very important to me that sometimes gets lost when thinking about the tooling and that is the human element and the importance of communication, existing relationships, and rapport. The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating but I have other coworkers that can slam my code and even pepper in insults but our relationship lets me read it in the jocular and secretly constructive way it is intended. The code is the same way and there was a great comment higher up about an ignored lint rule being an indicator of someone thinking about it and making a judgement call that this time the rule shouldn't apply. I have SOME peers where that my reading of that line of code and others where my assumption is the opposite and they were just lazy or didn't know how to do it "the right way". It's the same line of code! I don't know how tooling will ever replace this part so I think it's important to keep it as the bedrock of any collaboration process.
    - chipdart 2y ago
      > The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating (...) Posting PR comments doesn't prevent you from accepting the PR. You can post a torrent of nit comments and still approve it so that minor issues don't turn into blockers.
PhilipRoman 2y ago
IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves. Ping-pong with comments like "add empty line" is a waste of time for everyone.
- paiute 2y ago
  I like this idea, it has the bonus of revealing how much the reviewer actually cares. I bet half the things they wouldn’t take the time to fix themselves.
  - chipdart 2y ago
    > I like this idea, it has the bonus of revealing how much the reviewer actually cares. And now you contributed to turn your team culture into shit, with a toxic mix of PR creators purposely shitting on the team's style guide and PR reviewers who have to constantly post follow-up commits to your work because you can't even put together and acceptable PR or clean up after yourself. What if anyone who posts a PR addresses feedback anyone posts on their work and addresses them until you get the necessary and sufficient approvals?
- yjftsjthsd-h 2y ago
  Not just stylistic choices - I absolutely love the way ex. gitlab lets you do a suggested change in a comment, precisely because it lets me do all the work and the MR owner just has to click to accept if they agree. That lowers the bar for what I'm willing to bring up in the first place (though I personally still don't like to comment on matters of style or taste) while making it less work for everyone, which makes it much easier to give useful suggestions:)
  - mixmastamyk 2y ago
    Is that new? I don’t remember that feature, working with gitlab a few months back.
    - chipdart 2y ago
herpdyderp 2y ago
This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a reflection on the code review process being broken.
- SketchySeaBeast 2y ago
  Yeah, if you care about it enough to stop a PR because of something and that something can be automated, automate it.
- johannes1234321 2y ago
  There is a lot of stuff a linter can't do, which however are stylistic choices some people care a lot about. Naming being a big one. Which terms to abbreviate how etc. Also there are a lot of things where automatic highlighting is possible, but any rule needs exceptions which a re subjective (nesting, usage of "raw" looos, early exit, ...) Trying to get some consistency there can be good. And teaching new team members the "habits and traditions" may simplify long term maintenance. However could review tools aren't a good place for distinguishing between "hey, this is good, but I'd like this slightly different" from "there is an actual issue" but that has to be solved somehow by communication. Which often isn't the best skill for engineers.
  - chipdart 2y ago
    > There is a lot of stuff a linter can't do (...) Whatever a linter can't do, it's not worth doing. > Naming being a big one. You're confusing personal opinions over how to name things with concrete style issues.
- roland35 2y ago
  Yes exactly! Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. Or if that is too much (no shame!), just accept style differences.
  - chipdart 2y ago
OptionOfT 2y ago
This is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }
- hibikir 2y ago
  That's also why I dislike it: The mind that will go apeshit over trivial style nonsense will still provide unhelpful feedback on other topics... but it will become far less obvious that their comments should be downgraded. Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at making the code regular
  - Jare 2y ago
    > it will become far less obvious that their comments should be downgraded. I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.
    - Brian_K_White 2y ago
      It is obvious that a style comment is of no significance, and that the person who made it chose to spend time and personal capital on something of no significance. It is not obvious that an actual code change is of poor quality. To show that takes infinitely more experience, work to analyse all the direct and indirect ramifications of both the original and proposed approaches, capacity to push back and decide that your own engineering judgement is equal or better than whoever is purporting to correct you, willingness to suffer everyone else accusing you of arrogance for that on top. Even with all of that, it's a lot harder to prove the valueless comment is valueless because the more you know, the more you know that practically every single line of code could be done 30 other ways with some valid argument for each one. It's completely insidious both for the junior and the senior. The junior has no way to know the junk comment is junk. So they internalize the comment and everyone is worse for it. The senior has it almost worse. They know enough to know they don't know everything and the comment might be valid, so they put in all kinds of work to try to figure out if they actually missed something, did they have point etc. Maybe in the end the code doesn't end up as bad as the junior just rolling over, but it sucked for everyone and the challenge was not really an intentional productive crucible, it was just a douche that everyone had to waste time and energy taking seriously. Only someone who actually is arrogant (whatever level, whichever side of the pr) has it easy. Again bad for everyone except them maybe.
onion2k 2y ago
It says PR comments benefit the reviewer in some way, and they're not always left to improve the code or to help the person who opened the PR. For example, if you work in a culture where being seen to leave PR comments is seen as positive regardless of the quality of them, or even that code review is a tracked metric that contributes to your performance review. That culture will turn even the most conscientious dev into a monster.
chipdart 2y ago
> Most of it was stylistic nitpicking. One man's "stylistic nitpicking" is another man's violation of the company's official coding guidelines. Does the company adopted a linter? If so,why has the new hire not applied it before posting the PR? Imagine a new hire throwing a hissy fit because even though the whole company writes code in PascalCase he feels that snake_case is better. Imagine a new hire decided that tabs are better than spaces, and proceeded to reformat 30% of a source file leaving behind wonky indentation. Shall we tolerate code that has both just so that random internet people in a random online forum can rail against PRs? Or shall we post a comment in a PR pointing why the code should be reformatted?
- Larrikin 2y ago
  Using snake_case everywhere when you should be using camelCase should have been one or two comments at most. It's toxic to go through every use and comment
  - thiht 2y ago
    Toxic is a strong word. It’s useless, but not toxic, some people might think they’re making the reviewee’s work easier by flagging all the occurrences of an issue. Maybe they like that on their own PR. Maybe they never thought about it and think thoroughness is expected. If someone’s code review practice is bothering you, just mention it instead of labeling it as toxic.

znpy 2y ago

on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings). The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.

prh8 2y ago
"will approve after nitpicks" is the most asinine behavior
barbazoo 2y ago
Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.
dec0dedab0de 2y ago
* I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).* I always thought this was the best way. I wish these systems had a way to assign severity to comments, and urgency to the commit. If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.
- yjftsjthsd-h 2y ago
  Depending on your company culture, you can also just explicitly write something like "This is not a blocker, but I would suggest...". Of course if your culture uses explicit "press the button to unblock" then that's probably redundant.
  - to11mtm 2y ago
    Yeah I do similar and encourage colleagues/orgs to do the same.
  - erik_seaberg 2y ago
    Half of my comments were "nit: consider blah blah" where I want it on his radar, but I approved anyway and if he declines I'm fine with it.
- fmbb 2y ago
  I like https://blog.danlew.net/2020/04/15/the-logaf-scale/ https://blog.danlew.net/2020/04/15/the-logaf-scale/
to11mtm 2y ago
It's tough with spelling sometimes. In my current role I review a lot of PRs, they tend to be large due to the waterfall way things work... If I don't ask to fix the spelling now, It might not happen for a year or two. That said if it can be fixed in separate PR before release, that's fine. Really, it's best to have some terms or explicitness. For example, with my teams 'Nitpick' means I'm just being nitpicky, Doesn't have to change unless it's on the edge (and I'm explicit as to why it should change, i.e. I know the next thing will need the change anyway). "Consider" means It doesn't have to happen, but here's some food for thought. "PLZ FIX" is fairly self explanatory. Also, making sure management (especially for contract houses) knows that PRs are a 'judgement free zone' and should not be held against people for perf reviews etc; that should be collected by other peer feedback channels instead.
rileymat2 2y ago
In many dynamic languages, depending on scope, spelling is not a nitpick, it has long term mental costs and very well can cause bugs.
- vips7L 2y ago
  Spelling automatically should be highlighted by the ide too. Spelling mistakes means you’re blatantly ignoring warnings from the ide.
  - znpy 2y ago
    Please see my other comment. I meant misspelling of English words in the comments. Misspelled code will not compile and/or pass the tests so the pr won’t be accepted anyway.
- znpy 2y ago
  I should have been less ambiguous: i meant spelling of words on the English language, in comments. Good pr systems will perform a build of the code and run tests, so if the mis-spelling is in the code the build will fail anyway.
- Aeolun 2y ago
  Sorta, but it’s almost never worth blocking the PR over. Just ask to fix and approve. If it doesn’t happen and you come across it later just fix it.

bluefirebrand 2y ago

Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it

barbazoo 2y ago
Oh 100%. Discussing style within a PR unrelated to changing the styling rules seems like a waste of time.
deleted 2y ago
[deleted]
DanHulton 2y ago
This literally what the article says.
InvaderFizz 2y ago
I agree. Even within my own org there is some nitpicking, but it's almost always about style consistency for our shared codebase, which is valid. If your PR doesn't pass lint checks, it doesn't get merged. And the only reason it would fail the lint checks is if your pre-commit hooks didn't fire. There is no argument of 2,4,8 space vs tabs, because the code you commit is run through the linter. Write however you want for the things that don't matter, the formatter always wins.
chuckhend 2y ago
Automating this with linter and formatter is great. It moves the argument over style and format to a one liner change to a lint config instead of mingling it with the with the main code change.
- bluefirebrand 2y ago
  It also hopefully only happens once. If you continue to have people bringing up arguments over the linter and formatter after an initial agreement is made, then you can talk to those people

js8 2y ago

I think a good general advice when training somebody is - focus them only on the biggest flaw at the time. That's how I would proceed with a "bad" PR. I would even accepts smaller issues (style) when there is a bigger issue to fix. People might eventually outgrow it.

eddd-ddde 2y ago
For me it's the opposite. If I'm making a PR, please point out ALL of the nits you can even think about. My next PR is bound to have at most 25% of that since by now I get more about what the team values and considers important.
- collingreen 2y ago
  This highlights something very important to me that sometimes gets lost when thinking about the tooling and that is the human element and the importance of communication, existing relationships, and rapport. The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating but I have other coworkers that can slam my code and even pepper in insults but our relationship lets me read it in the jocular and secretly constructive way it is intended. The code is the same way and there was a great comment higher up about an ignored lint rule being an indicator of someone thinking about it and making a judgement call that this time the rule shouldn't apply. I have SOME peers where that my reading of that line of code and others where my assumption is the opposite and they were just lazy or didn't know how to do it "the right way". It's the same line of code! I don't know how tooling will ever replace this part so I think it's important to keep it as the bedrock of any collaboration process.
  - chipdart 2y ago
    > The PR feedback I give and receive is extremely influenced by the relationship I have with the person - for some PRs any nit pick feels frustrating (...) Posting PR comments doesn't prevent you from accepting the PR. You can post a torrent of nit comments and still approve it so that minor issues don't turn into blockers.

PhilipRoman 2y ago

IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves. Ping-pong with comments like "add empty line" is a waste of time for everyone.

paiute 2y ago
I like this idea, it has the bonus of revealing how much the reviewer actually cares. I bet half the things they wouldn’t take the time to fix themselves.
- chipdart 2y ago
  > I like this idea, it has the bonus of revealing how much the reviewer actually cares. And now you contributed to turn your team culture into shit, with a toxic mix of PR creators purposely shitting on the team's style guide and PR reviewers who have to constantly post follow-up commits to your work because you can't even put together and acceptable PR or clean up after yourself. What if anyone who posts a PR addresses feedback anyone posts on their work and addresses them until you get the necessary and sufficient approvals?
yjftsjthsd-h 2y ago
Not just stylistic choices - I absolutely love the way ex. gitlab lets you do a suggested change in a comment, precisely because it lets me do all the work and the MR owner just has to click to accept if they agree. That lowers the bar for what I'm willing to bring up in the first place (though I personally still don't like to comment on matters of style or taste) while making it less work for everyone, which makes it much easier to give useful suggestions:)
- mixmastamyk 2y ago
  Is that new? I don’t remember that feature, working with gitlab a few months back.
  - chipdart 2y ago

herpdyderp 2y ago

This kind of feedback has largely been solved, for me / my team at least, with linters and formatters (as mentioned in the article). So I'd say it is still a reflection on the code review process being broken.

SketchySeaBeast 2y ago
Yeah, if you care about it enough to stop a PR because of something and that something can be automated, automate it.
johannes1234321 2y ago
There is a lot of stuff a linter can't do, which however are stylistic choices some people care a lot about. Naming being a big one. Which terms to abbreviate how etc. Also there are a lot of things where automatic highlighting is possible, but any rule needs exceptions which a re subjective (nesting, usage of "raw" looos, early exit, ...) Trying to get some consistency there can be good. And teaching new team members the "habits and traditions" may simplify long term maintenance. However could review tools aren't a good place for distinguishing between "hey, this is good, but I'd like this slightly different" from "there is an actual issue" but that has to be solved somehow by communication. Which often isn't the best skill for engineers.
- chipdart 2y ago
  > There is a lot of stuff a linter can't do (...) Whatever a linter can't do, it's not worth doing. > Naming being a big one. You're confusing personal opinions over how to name things with concrete style issues.
roland35 2y ago
Yes exactly! Reviews should not waste time with format - I would take the hour to set that up in CI and never worry about it again. Or if that is too much (no shame!), just accept style differences.
- chipdart 2y ago

OptionOfT 2y ago

This is why I love things like cargo fmt / go fmt / eslint / etc. No discussions of whether if (foo) return; is valid, or we should do if (foo) { return; }

hibikir 2y ago
That's also why I dislike it: The mind that will go apeshit over trivial style nonsense will still provide unhelpful feedback on other topics... but it will become far less obvious that their comments should be downgraded. Formatters also do a poor job if your language is flexible and expressive. Great for go, but if your language is the kind that easily supports internal DSLs, formatters are not even helpful at making the code regular
- Jare 2y ago
  > it will become far less obvious that their comments should be downgraded. I'm curious why you think this would happen... I would imagine that their comments will now have be to about things that matter, and if they are unhelpful they will stand out more.
  - Brian_K_White 2y ago
    It is obvious that a style comment is of no significance, and that the person who made it chose to spend time and personal capital on something of no significance. It is not obvious that an actual code change is of poor quality. To show that takes infinitely more experience, work to analyse all the direct and indirect ramifications of both the original and proposed approaches, capacity to push back and decide that your own engineering judgement is equal or better than whoever is purporting to correct you, willingness to suffer everyone else accusing you of arrogance for that on top. Even with all of that, it's a lot harder to prove the valueless comment is valueless because the more you know, the more you know that practically every single line of code could be done 30 other ways with some valid argument for each one. It's completely insidious both for the junior and the senior. The junior has no way to know the junk comment is junk. So they internalize the comment and everyone is worse for it. The senior has it almost worse. They know enough to know they don't know everything and the comment might be valid, so they put in all kinds of work to try to figure out if they actually missed something, did they have point etc. Maybe in the end the code doesn't end up as bad as the junior just rolling over, but it sucked for everyone and the challenge was not really an intentional productive crucible, it was just a douche that everyone had to waste time and energy taking seriously. Only someone who actually is arrogant (whatever level, whichever side of the pr) has it easy. Again bad for everyone except them maybe.

onion2k 2y ago

It says PR comments benefit the reviewer in some way, and they're not always left to improve the code or to help the person who opened the PR. For example, if you work in a culture where being seen to leave PR comments is seen as positive regardless of the quality of them, or even that code review is a tracked metric that contributes to your performance review. That culture will turn even the most conscientious dev into a monster.

chipdart 2y ago

> Most of it was stylistic nitpicking. One man's "stylistic nitpicking" is another man's violation of the company's official coding guidelines. Does the company adopted a linter? If so,why has the new hire not applied it before posting the PR? Imagine a new hire throwing a hissy fit because even though the whole company writes code in PascalCase he feels that snake_case is better. Imagine a new hire decided that tabs are better than spaces, and proceeded to reformat 30% of a source file leaving behind wonky indentation. Shall we tolerate code that has both just so that random internet people in a random online forum can rail against PRs? Or shall we post a comment in a PR pointing why the code should be reformatted?

Larrikin 2y ago
Using snake_case everywhere when you should be using camelCase should have been one or two comments at most. It's toxic to go through every use and comment
- thiht 2y ago
  Toxic is a strong word. It’s useless, but not toxic, some people might think they’re making the reviewee’s work easier by flagging all the occurrences of an issue. Maybe they like that on their own PR. Maybe they never thought about it and think thoroughness is expected. If someone’s code review practice is bothering you, just mention it instead of labeling it as toxic.