6 ms·

> The golden rule: value your reviewer’s time This goes the other way around too. I've had some PR's that put copious amounts of time in to make them as comple

> The golden rule: value your reviewer’s time This goes the other way around too. I've had some PR's that put copious amounts of time in to make them as complete and perfect as I could for the reviewer. Only to get them turned down for style reasons. If style is that important, make sure you document the rules or automate it (3. Automate the easy stuff). So contributors have a chance at figuring this out for themselves, without having to go back and forth. If the style comes down to personal preference you can apply rule 12. Award all ties to your reviewer, in reverse too. Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.

npsimons 6y ago
I honestly love that Go has a formatter included, and the style is specified. Similarly with black[0] for Python. I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. This goes for other tools too, like static checkers, linters, even CI. I've been on projects where anyone could run the whole CI pipeline (which included the aforementioned static checkers, unit tests, even a doxygen run to check for undocumented things) on their dev machine before submitting, and not waste anyone's time with things better done by a computer. Wish every project could be like that. [0] - https://black.readthedocs.io/en/stable/ https://black.readthedocs.io/en/stable/
- aequitas 6y ago
  > I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. I had the same thing. Also consistency and automation is what's really important, as it extinguishes every need for style discussions. Because you can trivially convert from the project style to personal styles and vice versa using things like Git attributes[0], as long as it is consistent. [0] https://git-scm.com/docs/gitattributes https://git-scm.com/docs/gitattributes
  - FalconSensei 6y ago
    yeah, same with me. That's why I love prettier. I care more about just having a pattern that's applied automatically, than I care about personal preferences.
- musingsole 6y ago
  Consistency for consistency's sake is not a virtue. The best solution to all the style discussion nonsense is an automated, mandated style. The better solution is the team being mature enough to only care about real problems and not gate development with personal preferences. There's something to be said for feeling comfortable in your space. The company's codebase is not your space.
  - npsimons 6y ago
    Consistency for readability is a virtue; did you miss that part? And it's not about "gating" development with "personal preferences", what a strawman. If someone can't be bothered to run the formatting tool included in the repo on their code before submitting, then I can't be bothered to review their submission. > The best solution to all the style discussion nonsense is an automated, mandated style. Yes, exactly what I said. Again, did you miss that?
    - markrages 6y ago
      Consistency doesn't always serve readability. In written English, you can convey the same basic facts with a hundred different sentences. A good author will choose the one that best conveys emphasis and subtle gradations of meaning to the attentive reader. Code is also written for humans to read, so the best written code will use all the available facilities of the language to convey meaning to the reader.
      - makapuf 6y ago
        Exactly Like english or any written language, generally books are typesetted against a bunch of general conventions. You dont choose if you get one or two spaces after a semicolon. Your book is not great because you wrote it in leetcase, you focus on the meaning, not the details.
        musingsole 6y ago
        You really do get to choose if you want 1 or 2 spaces or whatever formatting you want. Certain publishing houses might have style guides that they follow strictly and others less so, but all this just makes the formatting for a book a perfect metaphor to formatting code.
- rkuykendall-com 6y ago
  For JS/TS/etc, Prettier: https://prettier.io/ https://prettier.io/ I don't agree with all the formatting, but I also just don't care enough. It's sooooo nice running `yarn format` on a huge legacy codebase after adding prettier.
  - zmmmmm 6y ago
    ughh ... I've never seen HTML code uglier or harder to read than what came out of prettier on the code bases we have that it is used on.
- brobinson 6y ago
  The fact that gofmt removes whitespace between arithmetic operators drives me insane. Fortunately, rustfmt and black don't do that, and I've been largely able to avoid writing Go for the last year now.
bird_monster 6y ago
I have a rule that if stylistic decisions matter enough to reject a PR over, they also have to be enforced by a linter or code formatter. If they are that big a deal, we should enforce them via automation, not review. And if they're not that big a deal, do not put them in a code review.
- Tade0 6y ago
  It baffles me that this isn't the consensus. I've waged wars over documenting styling rules (some too complicated to be covered by a formatter - which by itself is a red flag) because I know that people often forget to enforce rules that they themselves have come up with.
zests 6y ago
> Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code. Or the reviewer that always asks for more tests. (Once I got asked to write a test that was unnecessary and almost impossible to write a test for. Think timing issues. My options were to push back or spend a significant amount of time writing a test that was not needed. As a junior that already pushed back more than a typical junior I made the best choice I thought I could make at the time.)
- aequitas 6y ago
  What really helps in the case of testing is lead by example. If the project itself provides a good test framework and examples of properly implemented tests it is easy for contributors to add tests as well. But asking people to figure this stuff out themselves is often a really big hurdle. Also parametrized tests. I find no shame in asking people to add a case to a existing parameterized testsuite when submitting an issue.
sequoia 6y ago
How I managed this on a team: If there is a style disagreement, documented style rules prevail. If there is a documented style rule, follow it. If there is not a documented style rule, you cannot block this PR with the style objection. This avoids really annoying issues of "I don't like how this is written [for style reasons]" in PR feedback. I'm not a mind reader, I don't whether you prefer for..in over Array.foreach or whatever, my PR shouldn't be blocked because of that. If you want new rule, go propose one and we can discuss & vote on it later but it doesn't apply to this PR. This avoids the issue of "style rules are determined by whoever has the greatest capacity for argument & is willing to block PRs over it." It goes to a discussion & vote and everyone can agree once then stop talking about it. :)
phendrenad2 6y ago
Exactly. Nothing I love more than working hard to make sure my PR meets the personal style of the colleague whom I expect will review it, only to have to have it reviewed by another colleague, with a mutually exclusive style, meaning I have to do it all over again, and then that reviewer goes on vacation and I get grilled by the expected reviewer on why I did it "that way". Sigh.
lmilcin 6y ago
Yeah... I have a lot of that discussion with my team, too. It seems people (reviewers) need to find something or they think they are not doing good job. Another irritating practice is treating opinions about style as commands -- basically commanding the author to change the style of the code to conform to the reviewer. It should be understood that every problem has many potentially correct solutions. The job of the reviewer isn't to assure the best correct solution or the solution they like, it is to verify that the one selected by author is in the set of correct solutions. If this makes sense...

npsimons 6y ago

I honestly love that Go has a formatter included, and the style is specified. Similarly with black[0] for Python. I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. This goes for other tools too, like static checkers, linters, even CI. I've been on projects where anyone could run the whole CI pipeline (which included the aforementioned static checkers, unit tests, even a doxygen run to check for undocumented things) on their dev machine before submitting, and not waste anyone's time with things better done by a computer. Wish every project could be like that. [0] - https://black.readthedocs.io/en/stable/ https://black.readthedocs.io/en/stable/

aequitas 6y ago
> I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. I had the same thing. Also consistency and automation is what's really important, as it extinguishes every need for style discussions. Because you can trivially convert from the project style to personal styles and vice versa using things like Git attributes[0], as long as it is consistent. [0] https://git-scm.com/docs/gitattributes https://git-scm.com/docs/gitattributes
- FalconSensei 6y ago
  yeah, same with me. That's why I love prettier. I care more about just having a pattern that's applied automatically, than I care about personal preferences.
musingsole 6y ago
Consistency for consistency's sake is not a virtue. The best solution to all the style discussion nonsense is an automated, mandated style. The better solution is the team being mature enough to only care about real problems and not gate development with personal preferences. There's something to be said for feeling comfortable in your space. The company's codebase is not your space.
- npsimons 6y ago
  Consistency for readability is a virtue; did you miss that part? And it's not about "gating" development with "personal preferences", what a strawman. If someone can't be bothered to run the formatting tool included in the repo on their code before submitting, then I can't be bothered to review their submission. > The best solution to all the style discussion nonsense is an automated, mandated style. Yes, exactly what I said. Again, did you miss that?
  - markrages 6y ago
    Consistency doesn't always serve readability. In written English, you can convey the same basic facts with a hundred different sentences. A good author will choose the one that best conveys emphasis and subtle gradations of meaning to the attentive reader. Code is also written for humans to read, so the best written code will use all the available facilities of the language to convey meaning to the reader.
    - makapuf 6y ago
      Exactly Like english or any written language, generally books are typesetted against a bunch of general conventions. You dont choose if you get one or two spaces after a semicolon. Your book is not great because you wrote it in leetcase, you focus on the meaning, not the details.
      - musingsole 6y ago
        You really do get to choose if you want 1 or 2 spaces or whatever formatting you want. Certain publishing houses might have style guides that they follow strictly and others less so, but all this just makes the formatting for a book a perfect metaphor to formatting code.
rkuykendall-com 6y ago
For JS/TS/etc, Prettier: https://prettier.io/ https://prettier.io/ I don't agree with all the formatting, but I also just don't care enough. It's sooooo nice running `yarn format` on a huge legacy codebase after adding prettier.
- zmmmmm 6y ago
  ughh ... I've never seen HTML code uglier or harder to read than what came out of prettier on the code bases we have that it is used on.
brobinson 6y ago
The fact that gofmt removes whitespace between arithmetic operators drives me insane. Fortunately, rustfmt and black don't do that, and I've been largely able to avoid writing Go for the last year now.

bird_monster 6y ago

I have a rule that if stylistic decisions matter enough to reject a PR over, they also have to be enforced by a linter or code formatter. If they are that big a deal, we should enforce them via automation, not review. And if they're not that big a deal, do not put them in a code review.

Tade0 6y ago
It baffles me that this isn't the consensus. I've waged wars over documenting styling rules (some too complicated to be covered by a formatter - which by itself is a red flag) because I know that people often forget to enforce rules that they themselves have come up with.

zests 6y ago

> Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code. Or the reviewer that always asks for more tests. (Once I got asked to write a test that was unnecessary and almost impossible to write a test for. Think timing issues. My options were to push back or spend a significant amount of time writing a test that was not needed. As a junior that already pushed back more than a typical junior I made the best choice I thought I could make at the time.)

aequitas 6y ago
What really helps in the case of testing is lead by example. If the project itself provides a good test framework and examples of properly implemented tests it is easy for contributors to add tests as well. But asking people to figure this stuff out themselves is often a really big hurdle. Also parametrized tests. I find no shame in asking people to add a case to a existing parameterized testsuite when submitting an issue.

sequoia 6y ago

How I managed this on a team: If there is a style disagreement, documented style rules prevail. If there is a documented style rule, follow it. If there is not a documented style rule, you cannot block this PR with the style objection. This avoids really annoying issues of "I don't like how this is written [for style reasons]" in PR feedback. I'm not a mind reader, I don't whether you prefer for..in over Array.foreach or whatever, my PR shouldn't be blocked because of that. If you want new rule, go propose one and we can discuss & vote on it later but it doesn't apply to this PR. This avoids the issue of "style rules are determined by whoever has the greatest capacity for argument & is willing to block PRs over it." It goes to a discussion & vote and everyone can agree once then stop talking about it. :)

phendrenad2 6y ago

Exactly. Nothing I love more than working hard to make sure my PR meets the personal style of the colleague whom I expect will review it, only to have to have it reviewed by another colleague, with a mutually exclusive style, meaning I have to do it all over again, and then that reviewer goes on vacation and I get grilled by the expected reviewer on why I did it "that way". Sigh.

lmilcin 6y ago

Yeah... I have a lot of that discussion with my team, too. It seems people (reviewers) need to find something or they think they are not doing good job. Another irritating practice is treating opinions about style as commands -- basically commanding the author to change the style of the code to conform to the reviewer. It should be understood that every problem has many potentially correct solutions. The job of the reviewer isn't to assure the best correct solution or the solution they like, it is to verify that the one selected by author is in the set of correct solutions. If this makes sense...