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

Enforce ~consistent~ either hash syntax #309

Merged
merged 3 commits into from
Sep 27, 2022

Conversation

schmijos
Copy link
Member

@schmijos schmijos commented Sep 26, 2022

EnforcedShorthandSyntax: always is the default setting in Rubocop even though Bozhidar himself doesn't like the option.

I suggest to change the default option to consistent because I can see the advantage of the shorthand syntax when passing on options to deeper levels. But in any other context I found myself to be confused by it.

This change allows hashes to be in shorthand syntax only if the whole hash uses it consistently. See rubocop/rubocop#10776.

Update:
I changed my mind. Let's follow standardrb. Be consistent yourselves if you see fit.

`EnforcedShorthandSyntax: always` is the default setting in Rubocop even though Bozhidar himself [doesn't like the option](https://batsov.com/articles/2022/01/20/bad-ruby-hash-value-omission/). I suggest to change the default option to `consistent` because I can see the advantage of the shorthand syntax when passing on options to deeper levels. But in any other context I found myself to be confused by it.

This change allows hashes to be in shorthand syntax only if the whole hash uses it consistently. See rubocop/rubocop#10776.
@schmijos schmijos self-assigned this Sep 26, 2022
@rnestler
Copy link
Member

though Bozhidar himself doesn't like the option.

After reading this, I think I'd even vote to disallow it completely. But consistent style is probably enough.

Copy link

@odthoma odthoma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, this is also what I use in my project

Copy link
Member

@coorasse coorasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can somebody explain to me the difference between consistent and either? Because I enabled the second one on vista.

@odthoma
Copy link

odthoma commented Sep 26, 2022

@coorasse

EnforcedShorthandSyntax: consistent
# bad
{foo: , bar: bar}

# good
{foo:, bar:}

# bad
{foo: , bar: baz}

# good
{foo: foo, bar: baz}
EnforcedShorthandSyntax: either
# good
{foo: foo, bar: bar}

# good
{foo:, bar:}

Copy link
Member

@coorasse coorasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to run it with consistent on vista currently and I get the following for example:

C: [Correctable] Style/HashSyntax: Omit the hash value.
        let(:section_instance) { section.new(card_request: card_request) }

I don't want to be forced to write let(:section_instance) { section.new(card_request: card_request) }.

At the same time it does not complain about some_method(:hello, :b, card_request:, logger:).

As of now, I'd prefer either to consistent

@coorasse
Copy link
Member

I found also an old discussion we had. Maybe irrelevant now: https://renuo.slack.com/archives/C013FEGN4D7/p1650550992375259

@coorasse
Copy link
Member

coorasse commented Sep 27, 2022

either is also the default on standardrb: https://github.com/testdouble/standard/blob/main/config/base.yml#L1358

standardrb/standard#375
but consistent was not available back then.

@schmijos
Copy link
Member Author

schmijos commented Sep 27, 2022

Switching to either, mainly because of this: rubocop/rubocop#10776 (comment)
and because of @coorasse mentioning standardrb.

@schmijos schmijos changed the title Enforce consistent hash syntax Enforce ~consistent~ either hash syntax Sep 27, 2022
@schmijos schmijos merged commit 42423a6 into main Sep 27, 2022
@schmijos schmijos deleted the enforce-consistent-hash-syntax branch September 27, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants