[Fix #1016] Add `Rails/RedundantActiveRecordAllMethod` cop #1020

masato-bkn · 2023-06-17T11:46:53Z

I'd like to add Rails/RedundantAll cop.
This cop detects redundant all used as a receiver for Active Record query method except user defined scope.

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.

koic · 2023-07-19T04:58:30Z

@masato-bkn ping.

masato-bkn · 2023-07-19T09:46:07Z

@koic

If this is a new cop, consider making a corresponding update to the Rails Style Guide.

Sorry, I realized I should have made the suggestion to the Rails Style Guide first. I will contact you again once I have worked on it.

masato-bkn · 2023-07-19T14:07:13Z

rubocop/rails-style-guide#339
I have created an issue about redundant all in the Rails Style Guide.

koic · 2023-06-23T02:20:52Z

lib/rubocop/cop/rails/redundant_all.rb

+      #   # good
+      #   User.order(:created_at)
+      #   User.find(id)
+      class RedundantAll < Base


RedundantAll might be too abstract. How about RedundantActiveRecordAllMethod?

Suggested change

class RedundantAll < Base

class RedundantActiveRecordAllMethod < Base

koic · 2023-06-23T02:20:55Z

lib/rubocop/cop/rails/redundant_all.rb

+        include ActiveRecordHelper
+        extend AutoCorrector
+
+        MSG = 'Redundant `all` detected'


The period is forgotten.

Suggested change

MSG = 'Redundant `all` detected'

MSG = 'Redundant `all` detected.'

koic · 2023-06-23T02:20:58Z

lib/rubocop/cop/rails/redundant_all.rb

+          return unless QUERYING_METHODS.include?(query_node.method_name)
+          return if node.receiver.nil? && !inherit_active_record_base?(node)
+
+          all_range = node.loc.selector


Suggested change

all_range = node.loc.selector

range_of_all_method = node.loc.selector

koic · 2023-06-23T02:21:00Z

spec/rubocop/cop/rails/redundant_all_spec.rb

+    'without(user)'
+  ]
+
+  describe '::QUERYING_METHODS' do


Suggested change

describe '::QUERYING_METHODS' do

describe "#{described_class}::QUERYING_METHODS" do

koic · 2023-06-23T02:21:01Z

spec/rubocop/cop/rails/redundant_all_spec.rb

+  sources = [
+    'and(User.where(age: 30))',
+    'annotate("selecting id")',
+    'any?',
+    'average(:age)',
+    'calculate(:average, :age)',
+    'count',
+    'create_or_find_by(name: name)',
+    'create_or_find_by!(name: name)',
+    'create_with(name: name)',
+    'delete_all',
+    'delete_by(id: id)',
+    'destroy_all',
+    'destroy_by(id: id)',
+    'distinct',
+    'eager_load(:posts)',
+    'except(:order)',
+    'excluding(user)',
+    'exists?',
+    'extending(Pagination)',
+    'extract_associated(:posts)',
+    'fifth',
+    'fifth!',
+    'find(id)',
+    'find_by(name: name)',
+    'find_by!(name: name)',
+    'find_each(&:do_something)',
+    'find_in_batches(&:do_something)',
+    'find_or_create_by(name: name)',
+    'find_or_create_by!(name: name)',
+    'find_or_initialize_by(name: name)',
+    'find_sole_by(name: name)',
+    'first',
+    'first!',
+    'first_or_create(name: name)',
+    'first_or_create!(name: name)',
+    'first_or_initialize(name: name)',
+    'forty_two',
+    'forty_two!',
+    'fourth',
+    'fourth!',
+    'from("users")',
+    'group(:age)',
+    'having("AVG(age) > 30")',
+    'ids',
+    'in_batches(&:do_something)',
+    'in_order_of(:id, ids)',
+    'includes(:posts)',
+    'invert_where',
+    'joins(:posts)',
+    'last',
+    'last!',
+    'left_joins(:posts)',
+    'left_outer_joins(:posts)',
+    'limit(n)',
+    'lock',
+    'many?',
+    'maximum(:age)',
+    'merge(users)',
+    'minimum(:age)',
+    'none',
+    'none?',
+    'offset(n)',
+    'one?',
+    'only(:order)',
+    'optimizer_hints("SeqScan(users)", "Parallel(users 8)")',
+    'or(User.where(age: 30))',
+    'order(:created_at)',
+    'pick(:id)',
+    'pluck(:age)',
+    'preload(:posts)',
+    'readonly',
+    'references(:posts)',
+    'reorder(:created_at)',
+    'reselect(:age)',
+    'rewhere(id: ids)',
+    'second',
+    'second!',
+    'second_to_last',
+    'second_to_last!',
+    'select(:age)',
+    'sole',
+    'strict_loading',
+    'sum(:age)',
+    'take(n)',
+    'take!',
+    'third',
+    'third!',
+    'third_to_last',
+    'third_to_last!',
+    'touch_all',
+    'unscope(:order)',
+    'update_all(name: name)',
+    'where(id: ids)',
+    'without(user)'
+  ]


Can you use let instead of local variable assignment?

koic · 2023-06-23T02:21:03Z

spec/rubocop/cop/rails/redundant_all_spec.rb

+    it 'contains all of the querying methods' do
+      methods = sources.map { |s| s.split('(')[0].to_sym }
+      expect(described_class::QUERYING_METHODS).to eq(methods)
+    end


Suggested change

it 'contains all of the querying methods' do

methods = sources.map { |s| s.split('(')[0].to_sym }

expect(described_class::QUERYING_METHODS).to eq(methods)

end

let(:querying_methods) { sources.map { |s| s.split('(')[0].to_sym } }

it 'contains all of the querying methods' do

expect(described_class::QUERYING_METHODS).to eq(querying_methods)

end

koic · 2023-06-23T02:21:06Z

spec/rubocop/cop/rails/redundant_all_spec.rb

+      RUBY
+    end
+
+    it 'does not register an offense when not using defined methods in ::QUERYING_METHODS' do


::QUERYING_METHODS seems like cbase. Can you update ActiveRecord::Querying::QUERYING_METHODS or RuboCop::Cop::Rails::RedundantAll::QUERYING_METHODS (you intend) ?

masato-bkn · 2023-08-23T01:51:34Z

@koic
Thank you for your review. I'll make the necessary changes.

masato-bkn · 2023-08-25T02:46:39Z

@koic
I've made several changes, including addressing the points you raised.
If it looks good, I'll squash the commits into one.

koic · 2023-08-25T03:05:42Z

config/default.yml

@@ -775,6 +775,12 @@ Rails/ReadWriteAttribute:
  Include:
    - app/models/**/*.rb

+Rails/RedundantActiveRecordAllMethod:
+  Description: Detect redundant `all` used as a receiver for Active Record query methods.
+  Enabled: true


Suggested change

Enabled: true

Enabled: pending

I've addressed it.

koic · 2023-08-25T03:05:44Z

lib/rubocop/cop/rails/redundant_active_record_all_method.rb

+          return unless QUERYING_METHODS.include?(query_node.method_name)
+          return if node.receiver.nil? && !inherit_active_record_base?(node)
+
+          range_of_all_method = node.loc.selector


It should also be able to handle the following cases:
rubocop/rails-style-guide#340 (comment)

I've already addressed cases where an Active Record query method follows all, as shown below:

# bad current_user.likes.all.order(:created_at) # good current_user.likes.order(:created_at)

https://github.com/rubocop/rubocop-rails/pull/1020/files#diff-8d2292cab6c23118ba7721e07148d49a27043e9202c75084bd49b25053a1575eR216

Should I also consider addressing cases where the receiver is a relation and no method follows all? Like this:

# bad current_user.likes.all # good current_user.likes

Oops, this is my mistake. No problem!

koic · 2023-08-26T19:26:23Z

config/default.yml

@@ -775,6 +775,12 @@ Rails/ReadWriteAttribute:
  Include:
    - app/models/**/*.rb

+Rails/RedundantActiveRecordAllMethod:
+  Description: Detect redundant `all` used as a receiver for Active Record query methods.


rubocop/rails-style-guide#340 has been merged. Can you add the Style Guide URL and squash your commits into one?

Suggested change

Description: Detect redundant `all` used as a receiver for Active Record query methods.

Description: Detect redundant `all` used as a receiver for Active Record query methods.

StyleGuide: 'https://rails.rubystyle.guide/#redundant-all'

I've addressed it and squashed the commits!

koic · 2023-08-28T15:24:52Z

Thanks!

masato-bkn changed the title ~~[Fix: #1016] Add Rails/RedundantAll cop~~ [Fix #1016] Add Rails/RedundantAll cop Jun 17, 2023

masato-bkn force-pushed the add_rails_redundant_all branch from 4a16e3a to b2f9b5f Compare June 18, 2023 07:14

masato-bkn force-pushed the add_rails_redundant_all branch 3 times, most recently from 4aa8ede to e6a3ff5 Compare August 11, 2023 09:54

koic reviewed Aug 22, 2023

View reviewed changes

masato-bkn force-pushed the add_rails_redundant_all branch from 3819267 to 5ae8160 Compare August 25, 2023 02:44

koic reviewed Aug 25, 2023

View reviewed changes

masato-bkn changed the title ~~[Fix #1016] Add Rails/RedundantAll cop~~ [Fix #1016] Add Rails/RedundantActiveRecordAllMethod cop Aug 25, 2023

koic reviewed Aug 26, 2023

View reviewed changes

masato-bkn force-pushed the add_rails_redundant_all branch 2 times, most recently from 519290c to adec01a Compare August 27, 2023 02:35

[Fix rubocop#1016] Add Rails/RedundantActiveRecordAllMethod cop

e9a7221

masato-bkn force-pushed the add_rails_redundant_all branch from adec01a to e9a7221 Compare August 27, 2023 02:39

koic merged commit acafbcb into rubocop:master Aug 28, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #1016] Add `Rails/RedundantActiveRecordAllMethod` cop #1020

[Fix #1016] Add `Rails/RedundantActiveRecordAllMethod` cop #1020

masato-bkn commented Jun 17, 2023 •

edited

Loading

koic commented Jul 19, 2023

masato-bkn commented Jul 19, 2023

masato-bkn commented Jul 19, 2023 •

edited

Loading

koic Jun 23, 2023

koic Jun 23, 2023

koic Jun 23, 2023

koic Jun 23, 2023

koic Jun 23, 2023

koic Jun 23, 2023

koic Jun 23, 2023

masato-bkn commented Aug 23, 2023

masato-bkn commented Aug 25, 2023

koic Aug 25, 2023

masato-bkn Aug 25, 2023

koic Aug 25, 2023

masato-bkn Aug 25, 2023 •

edited

Loading

koic Aug 25, 2023

koic Aug 26, 2023

masato-bkn Aug 27, 2023

koic commented Aug 28, 2023

	class RedundantAll < Base
	class RedundantActiveRecordAllMethod < Base

	MSG = 'Redundant `all` detected'
	MSG = 'Redundant `all` detected.'

	all_range = node.loc.selector
	range_of_all_method = node.loc.selector

	describe '::QUERYING_METHODS' do
	describe "#{described_class}::QUERYING_METHODS" do

	Description: Detect redundant `all` used as a receiver for Active Record query methods.
	Description: Detect redundant `all` used as a receiver for Active Record query methods.
	StyleGuide: 'https://rails.rubystyle.guide/#redundant-all'

[Fix #1016] Add Rails/RedundantActiveRecordAllMethod cop #1020

[Fix #1016] Add Rails/RedundantActiveRecordAllMethod cop #1020

Conversation

masato-bkn commented Jun 17, 2023 • edited Loading

koic commented Jul 19, 2023

masato-bkn commented Jul 19, 2023

masato-bkn commented Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masato-bkn commented Aug 23, 2023

masato-bkn commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masato-bkn Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Aug 28, 2023

[Fix #1016] Add `Rails/RedundantActiveRecordAllMethod` cop #1020

[Fix #1016] Add `Rails/RedundantActiveRecordAllMethod` cop #1020

masato-bkn commented Jun 17, 2023 •

edited

Loading

masato-bkn commented Jul 19, 2023 •

edited

Loading

masato-bkn Aug 25, 2023 •

edited

Loading