Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable TrailingComma* cops #453

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link

These cops used to be configured with EnforcedStyleForMultiline: consistent_comma but it was changed in a7156b1 because it can cause some weird contortions, see: #133

I totally agree that this cop resulting in such horrible contorsion is broken, but I don't understand why that would justify to switch to a different style rather than to just disable the cop.

Trailing comma in multiline hashes & co have the major advantage of reducing diff noise when adding or removing an element.

NB: I'm not too sure if it's ok to open PRs to argue about style, but since this used to be the preferable style and was changed for a technicality, I hope it's ok.

@casperisfine
Copy link
Author

Also: I'm not very good with the rubocop codebase, but I wonder if it would be worth improving the upstream Cop so that it doesn't generate this kind of contraptions (I may try my hand at it).

These cops used to be configured with `EnforcedStyleForMultiline: consistent_comma`
but it was changed in standardrb@a7156b1
because it can cause some weird contortions, see: standardrb#133

I totally agree that this cop resulting in such horrible contorsion
is broken, but I don't understand why that would justify to switch
to a different style rather than to just disable the cop.

Trailing comma in multiline hashes & co have the major advantage of
reducing diff noise when adding or removing an element.
@searls
Copy link
Contributor

searls commented Aug 31, 2022

Hi @casperisfine, thank you for the contribution.

The context on why we chose to ditch trailing commas is broader than the issue linked above. Here's the issue that we tracked the changed to and the discussion in rubyfmt that initiated the conversation.

In truth, I only adopted trailing commas when I first published standard at @penelopezone's insistence and after a year or two of living with them, my frustration with them did not alleviate, so I was relieved by her reversal on that stance.

There are two reasons why I prefer no_comma:

  1. Standard is designed to specify a code style that is natural and obvious to write. The way I write Ruby code at this point is standards-compliant; it's very rare for the formatter to change anything on save for me at this point. We could only accomplish this by carefully selecting a configuration that wasn't made much easier by delegating to the autofixer (it's why we don't have any sort of alignment rules like aligning = or : characters in a multiline literal or expression, because later statements could change the formatting of an earlier line). While it's certainly possible to get in the habit of always tacking on a , to the end of every line, this was the only rule in the whole standard set that I couldn't train myself to adopt—probably because of a lifetime of only adding a , to prose when I had something to add afterward. Finishing an expression with a comma feels unnatural for that reason, I believe.

  2. The git churn issue strikes me as less compelling as more and more tooling will visually highlight actual changes and deemphasize unchanged text. For example, from this very PR it's not distracting at all to see this line change is nothing more than a comma change:

Screenshot 2022-08-31 at 1 25 05 PM

The reason we switched the rule instead of disabling it is because this is a major decision point that affects code readability pretty significantly and I felt it was important that Standard make a call one way or another. When you read the full base.yml, you'll see we have been pretty sparing in which cops we have chosen, but this was one case where encountering a codebase that mixed-and-matched styles would feel inconsistent and messy.

I realize a lot of this is subjective and being on the other side of the argument makes anything I say feel arbitrary. I think this is one issue is at least partly motivated by taste and aesthetics.

@searls searls closed this Aug 31, 2022
@searls searls added the rule change 👩‍⚖️ Suggest a style guide rule change label Aug 31, 2022
@casperisfine casperisfine deleted the multiline-comma branch September 1, 2022 06:58
@casperisfine
Copy link
Author

No worries, and thank you for the extra context I missed.

I off course totally disagree for various reason, but as you said: taste and aesthetics 😄

@searls
Copy link
Contributor

searls commented Sep 1, 2022

Thanks.

A word I didn't use in my description above was "consistency". One of my two internal mottos with Standard is "be nothing if not consistent", and that meant that once trailing commas started causing some real headaches for rubyfmt around parameter lists and some head-scratchers for data literals in Standard, the fact that no-trailing-commas could be confidently applied safely 100% of the time, it became appealing because it was the only bulletproof consistent rule we could apply in all cases.

@fables-tales
Copy link

fwiw rubyfmt has some objectively horrible syntax choices because it's the only way to be consistent. We chose this path and are sticking to it

@krystof-k
Copy link

We've just started using Standard Ruby, but this is definitely a reason to ditch it. Probably one of the most important cases for a linter is to enforce trailing commas to keep the Git history clean. I strongly disagree with the aesthetics argument, this is code, not art.

@fables-tales
Copy link

I assure you we thought about git diffs at the time and decided it wasn't important enough to override this decision :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule change 👩‍⚖️ Suggest a style guide rule change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants