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 #10698] Enhance Style/HashExcept to support array inclusion checks #10699

Merged
merged 2 commits into from Jun 20, 2022

Conversation

nobuyo
Copy link
Contributor

@nobuyo nobuyo commented Jun 7, 2022

Fixes #10698.

I'm not sure if in?, exclude? should be covered by RuboCop, since it is an extension by ActiveSupport.

# Note the actual behaviors because it is confusing.

require 'active_support/core_ext/enumerable'
require 'active_support/core_ext/object'

# except
{ foo: 1, bar: 2, baz: 3 }.except(:foo, :bar)
# => {:baz=>3}

# reject
{ foo: 1, bar: 2, baz: 3 }.reject { |k, v| k.in?(%i[foo bar]) }
{ foo: 1, bar: 2, baz: 3 }.reject { |k, v| %i[foo bar].include?(k) }
{ foo: 1, bar: 2, baz: 3 }.reject { |k, v| !%i[foo bar].exclude?(k) }
# => {:baz=>3}

# select
{ foo: 1, bar: 2, baz: 3 }.select { |k, v| !k.in?(%i[foo bar]) }
{ foo: 1, bar: 2, baz: 3 }.select { |k, v| !%i[foo bar].include?(k) }
{ foo: 1, bar: 2, baz: 3 }.select { |k, v| %i[foo bar].exclude?(k) }
# => {:baz=>3}

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@nobuyo nobuyo marked this pull request as draft Jun 7, 2022
@nobuyo nobuyo marked this pull request as ready for review Jun 8, 2022
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 10, 2022

I'm not sure if in?, exclude? should be covered by RuboCop, since it is an extension by ActiveSupport.

That's a tricky question. Normally I'd say "no", but ActiveSupport extensions are kind of special and most people use Rails. On the other hand they might have disabled some AS extension...

I don't think there's a high chance for false positives, though, so perhaps you can add some check if rubocop-rails is present and check for in? an exclude? or put this behind a config option.

@nobuyo nobuyo marked this pull request as draft Jun 10, 2022
@Darhazer
Copy link
Contributor

@Darhazer Darhazer commented Jun 10, 2022

Perhaps we need a configuration option whether rubocop cops to account for ActiveSupport methods, so people can just turn it on or off, and cops have common way to extend their own checks based on that.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 11, 2022

Perhaps we need a configuration option whether rubocop cops to account for ActiveSupport methods, so people can just turn it on or off, and cops have common way to extend their own checks based on that.

Keep in mind that outside of Rails you can require those in a very granular way, though. But I guess a single option will be fine for most people, as I assume it's rarely used outside of Rails.

@nobuyo nobuyo marked this pull request as ready for review Jun 12, 2022
@@ -3724,6 +3724,8 @@ Style/HashExcept:
that can be replaced with `Hash#except` method.
Enabled: pending
VersionAdded: '1.7'
VersionChanged: '<<next>>'
ActiveSupportEnabled: false
Copy link
Contributor Author

@nobuyo nobuyo Jun 12, 2022

Choose a reason for hiding this comment

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

I've added ActiveSupportEnabled option to switch checking methods extended by ActiveSupport.
The option name might have better alternatives 🤔

Copy link
Collaborator

@bbatsov bbatsov Jun 14, 2022

Choose a reason for hiding this comment

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

@nobuyo I like @Darhazer's idea to have some shared config option for this (perhaps a global config option?), so all the impacted cop can check for the same thing. The name of the option is fine, although if we want to be more specific I guess it should ActiveSupportExtensionsEnabled or however they call them these days. I'm fine with the current name as well.

@rubocop/rubocop-core any thoughts on this?

Copy link
Contributor Author

@nobuyo nobuyo Jun 14, 2022

Choose a reason for hiding this comment

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

Sorry I was misunderstanding. I'll start over with a new way (create a new global option?).

@nobuyo nobuyo marked this pull request as draft Jun 14, 2022
@nobuyo nobuyo force-pushed the enhance-hash-except branch 2 times, most recently from cbc49b1 to 8a30439 Compare Jun 17, 2022
config/default.yml Outdated Show resolved Hide resolved
@nobuyo nobuyo marked this pull request as ready for review Jun 17, 2022
@nobuyo nobuyo requested a review from bbatsov Jun 18, 2022
@bbatsov bbatsov merged commit 52c34a4 into rubocop:master Jun 20, 2022
30 checks passed
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 20, 2022

Looks good! We'll have to see which other cops can benefit from this setting and perhaps auto-set it to true in the presence of rubocop-rails, but overall we're on the right track.

@Mbarnes2022

This comment was marked as spam.

koic added a commit to koic/rubocop-rails that referenced this issue Jul 13, 2022
Follow up rubocop/rubocop#10699.

`ActiveSupportExtensionsEnabled` option is supported since RuboCop 1.31.0.
Please merge rubocop#729 first.
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.

5 participants