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.

Sign up for GitHub

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

Jump to bottom

Unexpected changes with named arguments in 1.6.0 from Style/HashSyntax #375

Closed
danielmorrison opened this issue Jan 6, 2022 · 14 comments
Closed

Unexpected changes with named arguments in 1.6.0 from Style/HashSyntax #375

danielmorrison opened this issue Jan 6, 2022 · 14 comments
Labels
approved 👍 rule change 👩‍⚖️ Suggest a style guide rule change

Comments

@danielmorrison
Copy link
Contributor

Looks like we have this new cop and I'm not a fan, so raising the issue here: Style/HashSyntax: Omit the hash value.

I'm getting code that looks like:

category = "foo"
Product.where(category: category)

Changing to:

category = "foo"
Product.where(category:)

On the one hand, I learned that this works! Cool that I can still have things to learn! On the other hand, it was pretty unexpected to me.

Looks like EnforcedShorthandSyntax: never is what I'd vote for.
@searls
Copy link
Contributor

searls commented Jan 7, 2022

Agree, this seems heavy-handed in practice. It's a neat trick but it doesn't make sense to force terseness on people, especially when it's so new. (I say this as someone who routinely supports way-since-EOL'd Rubies in my new code, so this seems like something I'll be late-to-the-party to).

@camilopayan, thoughts?

@searls searls added the rule change 👩‍⚖️ Suggest a style guide rule change label Jan 7, 2022
@whomwah
Copy link

whomwah commented Jan 7, 2022

It also causes problems if you are using a CI matrix of Ruby versions < 3.1 in CI.

@searls
Copy link
Contributor

searls commented Jan 7, 2022

Spoke with @camilopayan who agrees this rule is an overreach. We will cut a release ASAP that turns it off

@searls
Copy link
Contributor

searls commented Jan 7, 2022

also, for the record, I'm not going to enable the rule with EnforcedShorthandSyntax: never because I think this is a spiffy new feature that allows for terseness in lots of useful cases (like when an object exists to shovel lots attributes from one thing to the next), and it'd be a shame to ban its use in standard projects

@danielmorrison
Copy link
Contributor Author

Sounds great to me. I'm also fine not banning spiffy new things.

You guys are awesome. Keep up the great work!

@whomwah
Copy link

whomwah commented Jan 7, 2022

Thanks for the fast turnaround 👍

@searls
Copy link
Contributor

searls commented Jan 7, 2022

Well, shit. We don't have that option. I hadn't realized this was a tack-on to the (otherwise very important) Style/HashSyntax cop, and there's no option to be unopinionated about this

Screen Shot 2022-01-07 at 10 37 50

.

Paging @koic & @bbatsov for context here: I'd really wish for an option to say EnforcedShorthandSyntax: either so that neither way is enforced, while still requiring Ruby 1.9 style hash syntax. Do you think you could accommodate for that as an option in RuboCop?

@bbatsov
Copy link

bbatsov commented Jan 8, 2022 •
edited
Loading

Yeah, of course we can add either or rather none; we just didn't consider that someone might not care about this. You can find some context about our decision to fuse this check with the existing cop here rubocop/rubocop#10289 We'll address this in the next RuboCop release (most likely some time next week).

@searls
Copy link
Contributor

searls commented Jan 8, 2022

Thanks @bbatsov, I appreciate it! Please choose whatever name is more conventional, but either sounds good to me.

koic added a commit to koic/rubocop that referenced this issue Jan 8, 2022
@koic
Support EnforcedShorthandSyntax: either for Style/HashSyntax
2f54503 
Follow up to standardrb/standard#375 (comment).

This PR supports `EnforcedShorthandSyntax: either` option for `Style/HashSyntax`.
It accepts both shorthand and explicit use of hash literal value.

```ruby
# good
{foo: foo, bar: bar}

# good
{foo:, bar:}
```

I think that we can change the default enforced style in response to future feedback
and discussion. Anyway at this point it only adds the new style.
@koic koic mentioned this issue Jan 8, 2022
Merged
8 tasks
@koic
Copy link
Contributor

koic commented Jan 8, 2022

Yeah, I've opened rubocop/rubocop#10351 to support the new option.

bbatsov pushed a commit to rubocop/rubocop that referenced this issue Jan 9, 2022
@koic @bbatsov
Support EnforcedShorthandSyntax: either for Style/HashSyntax
1078425 
Follow up to standardrb/standard#375 (comment).

This PR supports `EnforcedShorthandSyntax: either` option for `Style/HashSyntax`.
It accepts both shorthand and explicit use of hash literal value.

```ruby
# good
{foo: foo, bar: bar}

# good
{foo:, bar:}
```

I think that we can change the default enforced style in response to future feedback
and discussion. Anyway at this point it only adds the new style.
@bbatsov
Copy link

bbatsov commented Jan 18, 2022

@searls The new RuboCop release (1.25) that features either is out.

@searls
Copy link
Contributor

searls commented Jan 18, 2022

Thank you @bbatsov! Just saw the release notification ❤️

@kaka-ruto
Copy link

You guys are sweet!

@ojgarcia ojgarcia mentioned this issue May 20, 2022
Closed
@coorasse coorasse mentioned this issue Sep 27, 2022
Merged
@jandudulski
Copy link
Contributor

jandudulski commented Oct 6, 2022 •
edited
Loading

RuboCop 1.35 added a new option - even better than either in my opinion: consistent will force you to be, well... consistent (so either shorthand or not but never both). WDYT about switching to this option?

@jandudulski jandudulski mentioned this issue Oct 6, 2022
Open
mctofu pushed a commit to jeffwidman/dependabot-core that referenced this issue Oct 7, 2022
@jeffwidman @mctofu
Allow but don't require omitting hash values
6dbec51 
Ruby 3.1 added some syntax sugar for omitting hash values in certain
cases. This results in more terse code, at the expense of readability.

Like any good style change, this is obviously bikeshed worthy, and has
generated a lot of controversy around the community:
* https://batsov.com/articles/2022/01/20/bad-ruby-hash-value-omission/
* standardrb/standard#375
* Shopify/ruby-style-guide#393
* https://github.com/rubocop/ruby-style-guide#hash-literal-values

Personally, I can see places where it's useful, but also places where it
just makes the Ruby a bit more inscrutable.

So set the Rubocop value to `either` to allow, but not require it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
approved 👍 rule change 👩‍⚖️ Suggest a style guide rule change
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@danielmorrison @whomwah @koic @searls @bbatsov @jandudulski @kaka-ruto