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 EnforcedShorthandSyntax in Style/HashSyntax #365

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

rafaelfranca
Copy link
Member

We should not be forcing the shorthand syntax because of a few reasons:

  1. That syntax doesn't always yields the best/clear code. It has
    surprising behavior specially with method calls using the key names.
  2. Ruby 3.1 shorthand syntax is still not supported by Sorbet.

We can fix 2, but not 1, so it is better to just allow people to chose
when to use the shorthand syntax in the context that makes sense.

We should not be forcing the shorthand syntax because of a few reasons:

1. That syntax doesn't always yields the best/clear code. It has
surprising behavior specially with method calls using the key names.
2. Ruby 3.1 shorthand syntax is still not supported by Sorbet.

We can fix 2, but not 1, so it is better to just allow people to chose
when to use the shorthand syntax in the context that makes sense.
@sambostock
Copy link
Contributor

What are your opinions on the no_mixed_keys setting?

@rafaelfranca
Copy link
Member Author

ruby19 seems better to me. If keys needs to be string, they still need to use hash rocket, so we sometimes need to mix syntax. Although that only happens if people are creating hashes that hold two different key types, which is so rare that I don't think we should care about that case.

@rafaelfranca rafaelfranca merged commit febf531 into main Mar 2, 2022
@rafaelfranca rafaelfranca deleted the rm-disable-EnforcedShorthandSyntax branch March 2, 2022 17:00
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems March 2, 2022 18:34 Inactive
@tomstuart
Copy link
Contributor

@rafaelfranca core currently has EnforcedShorthandSyntax: never in its RuboCop configuration, presumably to avoid the always default causing violations. If we upgrade rubocop-shopify to v2.5.0 in core, can we remove that configuration entirely (i.e. inherit either) or would you like to keep it pinned to the more conservative never?

@tomstuart
Copy link
Contributor

core currently has EnforcedShorthandSyntax: never in its RuboCop configuration, presumably to avoid the always default causing violations. If we upgrade rubocop-shopify to v2.5.0 in core, can we remove that configuration entirely (i.e. inherit either) or would you like to keep it pinned to the more conservative never?

I’ve speculatively done the former in Shopify/shopify#340042.

@andyw8
Copy link
Contributor

andyw8 commented Jan 30, 2023

Update: Ruby 3.1 shorthand syntax is now supported by Sorbet.

@sambostock
Copy link
Contributor

I suspect we'll still leave it up to the author to choose.

One thing I've noticed is that some issues & PRs cite this PR saying that "shorthand syntax should not be used", which as far as I can tell is incorrect. If I'm not mistaken, our stance is that we don't enforce shorthand syntax, but we do allow it, at the authors discretion, with the expectation that they go with what is clearest in that context.

x, y, z = string.split(",").map(&:to_f)
Point.new(x: x, y: y, z: z) # fine
Point.new(x:, y:, z:)       # also fine

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.

None yet

5 participants