From ab3e176984c8c98ab26563aa0a6abba6ffa699e9 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Mon, 25 Jul 2022 00:34:39 +0900 Subject: [PATCH] Deprecate `IgnoredMethods` option Follow up https://github.com/rubocop/rubocop/pull/10829. This PR deprecates `IgnoredMethods` option in integrate to `AllowedMethods` and `AllowedPatterns` option. It requires RuboCop 1.33.0 or higher. --- ...ange_deprecate_ignore_methods_parameter.md | 1 + config/default.yml | 26 ++++++++++ config/obsoletion.yml | 10 ++++ lib/rubocop/cop/rails/find_each.rb | 10 +++- rubocop-rails.gemspec | 2 +- spec/rubocop/cop/rails/find_each_spec.rb | 48 ++++++++++++++++++- 6 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 changelog/change_deprecate_ignore_methods_parameter.md diff --git a/changelog/change_deprecate_ignore_methods_parameter.md b/changelog/change_deprecate_ignore_methods_parameter.md new file mode 100644 index 0000000000..3dc28d6ea9 --- /dev/null +++ b/changelog/change_deprecate_ignore_methods_parameter.md @@ -0,0 +1 @@ +* [#750](https://github.com/rubocop/rubocop-rails/pull/750): Deprecate `IgnoredMethods` option in integrate to `AllowedMethods` and `AllowedPatterns` option. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index e827ebcaf0..fc34cc8a79 100644 --- a/config/default.yml +++ b/config/default.yml @@ -26,6 +26,24 @@ Lint/NumberConversion: # Add Rails' duration methods to the ignore list for `Lint/NumberConversion` # so that calling `to_i` on one of these does not register an offense. # See: https://github.com/rubocop/rubocop/issues/8950 + AllowedMethods: + - ago + - from_now + - second + - seconds + - minute + - minutes + - hour + - hours + - day + - days + - week + - weeks + - fortnight + - fortnights + - in_milliseconds + AllowedPatterns: [] + # Deprecated. IgnoredMethods: - ago - from_now @@ -410,6 +428,14 @@ Rails/FindEach: VersionChanged: '2.9' Include: - app/models/**/*.rb + AllowedMethods: + # Methods that don't work well with `find_each`. + - order + - limit + - select + - lock + AllowedPatterns: [] + # Deprecated. IgnoredMethods: # Methods that don't work well with `find_each`. - order diff --git a/config/obsoletion.yml b/config/obsoletion.yml index 9cfb451ff7..b304073aa6 100644 --- a/config/obsoletion.yml +++ b/config/obsoletion.yml @@ -5,3 +5,13 @@ # extracted: Rails/*: ~ + +# Cop parameters that have been changed +# Can be treated as a warning instead of a failure with `severity: warning` +changed_parameters: + - cops: Rails/FindEach + parameters: IgnoredMethods + alternatives: + - AllowedMethods + - AllowedPatterns + severity: warning diff --git a/lib/rubocop/cop/rails/find_each.rb b/lib/rubocop/cop/rails/find_each.rb index 73eb23fc45..4857c13891 100644 --- a/lib/rubocop/cop/rails/find_each.rb +++ b/lib/rubocop/cop/rails/find_each.rb @@ -13,11 +13,17 @@ module Rails # # good # User.all.find_each # - # @example IgnoredMethods: ['order'] + # @example AllowedMethods: ['order'] + # # good + # User.order(:foo).each + # + # @example AllowedPattern: [/order/] # # good # User.order(:foo).each class FindEach < Base include ActiveRecordHelper + include AllowedMethods + include AllowedPattern extend AutoCorrector MSG = 'Use `find_each` instead of `each`.' @@ -47,7 +53,7 @@ def ignored?(node) method_chain = node.each_node(:send).map(&:method_name) - (cop_config['IgnoredMethods'].map(&:to_sym) & method_chain).any? + method_chain.any? { |method_name| allowed_method?(method_name) || matches_allowed_pattern?(method_name) } end def active_model_error_where?(node) diff --git a/rubocop-rails.gemspec b/rubocop-rails.gemspec index 1757750915..e0639990a5 100644 --- a/rubocop-rails.gemspec +++ b/rubocop-rails.gemspec @@ -35,5 +35,5 @@ Gem::Specification.new do |s| # Rack::Utils::SYMBOL_TO_STATUS_CODE, which is used by HttpStatus cop, was # introduced in rack 1.1 s.add_runtime_dependency 'rack', '>= 1.1' - s.add_runtime_dependency 'rubocop', '>= 1.31.0', '< 2.0' + s.add_runtime_dependency 'rubocop', '>= 1.33.0', '< 2.0' end diff --git a/spec/rubocop/cop/rails/find_each_spec.rb b/spec/rubocop/cop/rails/find_each_spec.rb index bb43b38dde..d4d26303b0 100644 --- a/spec/rubocop/cop/rails/find_each_spec.rb +++ b/spec/rubocop/cop/rails/find_each_spec.rb @@ -118,8 +118,54 @@ class C < ActiveRecord::Base end end + context 'allowed methods' do + let(:cop_config) { { 'AllowedMethods' => %w[order lock], 'AllowedPatterns' => [], 'IgnoredMethods' => [] } } + + it 'does not register an offense when using order(...) earlier' do + expect_no_offenses('User.order(:name).each { |u| u.something }') + end + + it 'does not register an offense when using order(...) chained with other things' do + expect_no_offenses('User.order(:name).includes(:company).each { |u| u.something }') + end + + it 'does not register an offense when using lock earlier' do + expect_no_offenses('User.lock.each { |u| u.something }') + end + + it 'registers offense for methods not in `AllowedMethods`' do + expect_offense(<<~RUBY) + User.joins(:posts).each { |u| u.something } + ^^^^ Use `find_each` instead of `each`. + RUBY + end + end + + context 'allowed patterns' do + let(:cop_config) { { 'AllowedMethods' => [], 'AllowedPatterns' => [/order/, /lock/], 'IgnoredMethods' => [] } } + + it 'does not register an offense when using order(...) earlier' do + expect_no_offenses('User.order(:name).each { |u| u.something }') + end + + it 'does not register an offense when using order(...) chained with other things' do + expect_no_offenses('User.order(:name).includes(:company).each { |u| u.something }') + end + + it 'does not register an offense when using lock earlier' do + expect_no_offenses('User.lock.each { |u| u.something }') + end + + it 'registers offense for methods not in `AllowedPatterns`' do + expect_offense(<<~RUBY) + User.joins(:posts).each { |u| u.something } + ^^^^ Use `find_each` instead of `each`. + RUBY + end + end + context 'ignored methods' do - let(:cop_config) { { 'IgnoredMethods' => %w[order lock] } } + let(:cop_config) { { 'AllowedPatterns' => [], 'AllowedMethods' => [], 'IgnoredMethods' => %w[order lock] } } it 'does not register an offense when using order(...) earlier' do expect_no_offenses('User.order(:name).each { |u| u.something }')