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

[Fix #7641] Remove Style/BracesAroundHashParameters cop #7643

Merged

Conversation

pocke
Copy link
Collaborator

@pocke pocke commented Jan 11, 2020

Fix #7641

This pull request removes Style/BracesAroundHashParameters cop. Because keyword arguments behavior will be changed in Ruby 3, and has been warned in Ruby 2.7.
For more details, see #7641 and https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

This pull request removes the cop, and related code. Because we had hacks to avoid auto-correction generates wrong code by the cop and others.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 12, 2020

I'm guessing we'll also need to update the style guide.

@@ -208,196 +206,58 @@ def batch
let(:comma_style) do
'comma'
end
let(:expected_corrected_source) do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I only left Style/BracesAroundHashParameters is disabled context.

@@ -1010,39 +870,6 @@ def do_something
expect(IO.read('example.rb')).to eq(corrected)
end

it 'corrects complicated cases conservatively' do
Copy link
Collaborator Author

@pocke pocke Jan 13, 2020

Choose a reason for hiding this comment

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

The following test cases are for the removed cop, so we can remove it.

@pocke pocke force-pushed the Good-bye-Style/BracesAroundHashParameters branch 2 times, most recently from f8e25ab to b0d06ba Compare January 13, 2020 07:34
@pocke pocke marked this pull request as ready for review January 13, 2020 07:50
@pocke pocke changed the title [Fix #7641] [WIP] Remove Style/BracesAroundHashParameters cop [Fix #7641] Remove Style/BracesAroundHashParameters cop Jan 13, 2020
@koic
Copy link
Member

koic commented Jan 14, 2020

FYI, I opened #7648 to resolve ruby-head CI failure.

@koic
Copy link
Member

koic commented Jan 14, 2020

Can you add the Style/BracesAroundHashParameters cop to RuboCop::ConfigObsoletion::REMOVED_COPS?
https://github.com/rubocop-hq/rubocop/blob/v0.79.0/lib/rubocop/config_obsoletion.rb#L60

@pocke
Copy link
Collaborator Author

pocke commented Jan 14, 2020

Can you add the Style/BracesAroundHashParameters cop to RuboCop::ConfigObsoletion::REMOVED_COPS?

I added the cop to the removed cops list in bf79528.
I'll squash the commits into one if you'd like.

@koic
Copy link
Member

koic commented Jan 15, 2020

This looks good to me. Can you rebase with the latest master branch and squash your commits into one?

@pocke pocke force-pushed the Good-bye-Style/BracesAroundHashParameters branch from bf79528 to fd6dcac Compare January 15, 2020 02:04
@pocke
Copy link
Collaborator Author

pocke commented Jan 15, 2020

Rebased. Thanks for reviewing!

kevindew added a commit to kevindew/openapi3_parser that referenced this pull request Jan 16, 2020
This cop doesn't have relevance since the move in Ruby 2.7 towards
differentiating keyword arguments and hash arguments. It is due to be
removed from a future version of Rubocop: rubocop/rubocop#7643
@bbatsov bbatsov merged commit c3bc810 into rubocop:master Jan 21, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 21, 2020

Great work! Thanks!

@pocke pocke deleted the Good-bye-Style/BracesAroundHashParameters branch January 21, 2020 13:42
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 27, 2020

@pocke Btw, I was wondering if we actually should just revert the behaviour of the cop, so that people can migrate smoothly to Ruby 2.7+. Sorry that this came to my mind so late!

@pocke
Copy link
Collaborator Author

pocke commented Jan 27, 2020

Thanks for your comment. But I'm not sure I understand your comment correctly due to my poor English. 🙇‍♂️ I guess you meant you concerned removing the cop is the best way or not.

I guess we can re-implement the cop as meaningful for Ruby 2.7+. Some method can receive both of keyword arguments and a hash, for example, probably ActiveRecord's methods receive both.
In this case, we can choose one of the styles with the cop.
I'll try to implement the cop with the Ruby 3's static type analysis in https://github.com/pocke/rubocop-typed. But currently, I'm not sure the static type is helpful for the cop yet. But if we can implement it, I think it will be useful as a better replacement for Style/BracesAroundHashParameters cop.

And I guess re-implementing a useful replacement is difficult without static types. Because the replacement should be able to find a method definition from a method call, but it is difficult without types.
But perhaps we can implement it for limited cases. For example, focusing ActiveRecord#find_by(hash_or_keyword), because we can guess the method call is ActiveRecord's one easily. But I'm not sure it is useful enough...


Please tell me if I misunderstand something 🙇‍♂️ Thanks!

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 9, 2020

You've got some great ideas, but I had something simpler in mind - I was merely thinking that probably it makes sense to have a lint cop that warns people that their code needs to be updated for Ruby 2.7+ - just do the inverse of what it used to do in the past (suggest using braces instead of omitting them).

Anyways, that's definitely not a big deal. Just a random thought I had after merging the PR. Probably my suggestion is flawed in some way, as I haven't thought much about it.

Sharpie added a commit to Sharpie/pdk-templates that referenced this pull request May 4, 2020
The `Style/BracesAroundHashParameters` cop would flag "redundant" uses
of braces to create hashes in Ruby method arguments. Ruby 2.7 requires
these braces to disambiguate hash literals from keyword arguments and
will issue warnings. The cop has been removed from RuboCop v0.80.0.

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
See: rubocop/rubocop#7643
Sharpie added a commit to Sharpie/pdk-templates that referenced this pull request May 4, 2020
The `Style/BracesAroundHashParameters` cop would flag "redundant" uses
of braces to create hashes in Ruby method arguments. Ruby 2.7 requires
these braces to disambiguate hash literals from keyword arguments and
will issue warnings. The cop has been removed from RuboCop v0.80.0.

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
See: rubocop/rubocop#7643
Sharpie added a commit to Sharpie/pdk-templates that referenced this pull request May 4, 2020
The `Style/BracesAroundHashParameters` cop would flag "redundant" uses
of braces to create hashes in Ruby method arguments. Ruby 2.7 requires
these braces to disambiguate hash literals from keyword arguments and
will issue warnings. The cop has been removed from RuboCop v0.80.0.

See: https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/
See: rubocop/rubocop#7643
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.

Remove (or disable) Style/BracesAroundHashParameters
3 participants