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 false negatives for Rails/RedundantActiveRecordAllMethod when using POSSIBLE_ENUMERABLE_BLOCK_METHODS in a block #1382

Merged

Conversation

masato-bkn
Copy link
Contributor

@masato-bkn masato-bkn commented Oct 27, 2024

This PR fixes false negatives for Rails/RedundantActiveRecordAllMethod when methods in POSSIBLE_ENUMERABLE_BLOCK_METHODS are used in a block.

Expected Behavior

expect { subject }.to change { User.all.count }
                                    ^^^ Redundant `all` detected.

Actual Behavior

No offenses are registered.

Steps to reproduce the problem

Run bundle ex rubocop --only Rails/RedundantActiveRecordAllMethod on the code below:

expect { subject }.to change { User.all.count }

RuboCop version

1.67.0 (using Parser 3.3.5.0, rubocop-ast 1.32.3, analyzing as Ruby 3.2, running on ruby 3.2.0) [x86_64-darwin21]
  - rubocop-rails 2.27.0

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.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@masato-bkn masato-bkn changed the title Fix false negatives for Rails/RedundantActiveRecordAllMethod when m… Fix false negatives for Rails/RedundantActiveRecordAllMethod when using POSSIBLE_ENUMERABLE_BLOCK_METHODS in a block Oct 27, 2024
@masato-bkn masato-bkn force-pushed the fix-redundant_active_record_all_method branch 4 times, most recently from a97f810 to b8a039a Compare October 27, 2024 12:34
@masato-bkn masato-bkn marked this pull request as ready for review October 27, 2024 12:39
@@ -174,7 +174,7 @@ def possible_enumerable_block_method?(node)
parent = node.parent
return false unless POSSIBLE_ENUMERABLE_BLOCK_METHODS.include?(parent.method_name)

parent.parent&.block_type? || parent.parent&.numblock_type? || parent.first_argument&.block_pass_type?
parent.block_node || parent.first_argument&.block_pass_type?
Copy link
Member

Choose a reason for hiding this comment

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

block_node doesn't return a boolean value for predicate method. block_literal? seems more appropriate.

Suggested change
parent.block_node || parent.first_argument&.block_pass_type?
parent.block_literal? || parent.first_argument&.block_pass_type?

Copy link
Contributor Author

@masato-bkn masato-bkn Oct 29, 2024

Choose a reason for hiding this comment

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

Thank you for your review! I've fixed some issues.

@@ -0,0 +1 @@
* [#1382](https://github.com/rubocop/rubocop-rails/pull/1382): Fix false negatives for `Rails/RedundantActiveRecordAllMethod`. ([@masato-bkn][])
Copy link
Member

Choose a reason for hiding this comment

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

The change log could use a bit more information. For example, something like the following:

Suggested change
* [#1382](https://github.com/rubocop/rubocop-rails/pull/1382): Fix false negatives for `Rails/RedundantActiveRecordAllMethod`. ([@masato-bkn][])
* [#1382](https://github.com/rubocop/rubocop-rails/pull/1382): Fix false negatives for `Rails/RedundantActiveRecordAllMethod` when using `all` method in block. ([@masato-bkn][])

@masato-bkn masato-bkn force-pushed the fix-redundant_active_record_all_method branch from b8a039a to e42189c Compare October 29, 2024 11:24
…sing `all` method in block

This PR fixes false negatives for `Rails/RedundantActiveRecordAllMethod` when methods in `POSSIBLE_ENUMERABLE_BLOCK_METHODS` are used in a block as following:

```
expect { subject }.to change { User.all.count }
```
@masato-bkn masato-bkn force-pushed the fix-redundant_active_record_all_method branch from e42189c to 2d7e7a0 Compare October 29, 2024 11:26
@koic koic merged commit ad3287a into rubocop:master Oct 29, 2024
14 checks passed
@koic
Copy link
Member

koic commented Oct 29, 2024

Thanks!

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.

2 participants