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

Make Rails/RedundantReceiverInWithOptions and Rails/ReversibleMigration aware of numblock #754

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions .rubocop_todo.yml
Expand Up @@ -14,9 +14,6 @@ InternalAffairs/NodeDestructuring:
- 'lib/rubocop/cop/rails/relative_date_constant.rb'
- 'lib/rubocop/cop/rails/time_zone.rb'

InternalAffairs/NumblockHandler:
Enabled: false

# Offense count: 10
Metrics/AbcSize:
Max: 17
Expand Down
@@ -0,0 +1 @@
* [#754](https://github.com/rubocop/rubocop-rails/pull/754): Make `Rails/RedundantReceiverInWithOptions` and `Rails/ReversibleMigration` cops aware of numbered block parameter. ([@koic][])
2 changes: 1 addition & 1 deletion lib/rubocop/cop/mixin/index_method.rb
Expand Up @@ -6,7 +6,7 @@ module Cop
module IndexMethod # rubocop:disable Metrics/ModuleLength
RESTRICT_ON_SEND = %i[each_with_object to_h map collect []].freeze

def on_block(node)
def on_block(node) # rubocop:todo InternalAffairs/NumblockHandler
on_bad_each_with_object(node) do |*match|
handle_possible_offense(node, match, 'each_with_object')
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/action_filter.rb
Expand Up @@ -69,7 +69,7 @@ class ActionFilter < Base

RESTRICT_ON_SEND = FILTER_METHODS + ACTION_METHODS

def on_block(node)
def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
check_method_node(node.send_node)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/rake_environment.rb
Expand Up @@ -39,7 +39,7 @@ class RakeEnvironment < Base
(block $(send nil? :task ...) ...)
PATTERN

def on_block(node)
def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
task_definition?(node) do |task_method|
return if task_name(task_method) == :default
return if with_dependencies?(task_method)
Expand Down
54 changes: 28 additions & 26 deletions lib/rubocop/cop/rails/redundant_receiver_in_with_options.rb
Expand Up @@ -60,15 +60,6 @@ class RedundantReceiverInWithOptions < Base

MSG = 'Redundant receiver in `with_options`.'

def_node_matcher :with_options?, <<~PATTERN
(block
(send nil? :with_options
(...))
(args
$_arg)
$_body)
PATTERN

def_node_search :all_block_nodes_in, <<~PATTERN
(block ...)
PATTERN
Expand All @@ -78,29 +69,40 @@ class RedundantReceiverInWithOptions < Base
PATTERN

def on_block(node)
with_options?(node) do |arg, body|
return if body.nil?
return unless all_block_nodes_in(body).count.zero?

send_nodes = all_send_nodes_in(body)

if send_nodes.all? { |n| same_value?(arg, n.receiver) }
send_nodes.each do |send_node|
receiver = send_node.receiver
add_offense(receiver.source_range) do |corrector|
autocorrect(corrector, send_node)
end
end
return unless node.method?(:with_options)
return unless (body = node.body)
return unless all_block_nodes_in(body).count.zero?

send_nodes = all_send_nodes_in(body)
return unless redundant_receiver?(send_nodes, node)

send_nodes.each do |send_node|
receiver = send_node.receiver
add_offense(receiver.source_range) do |corrector|
autocorrect(corrector, send_node, node)
end
end
end

alias on_numblock on_block

private

def autocorrect(corrector, node)
corrector.remove(node.receiver.source_range)
corrector.remove(node.loc.dot)
corrector.remove(block_argument_range(node))
def autocorrect(corrector, send_node, node)
corrector.remove(send_node.receiver.source_range)
corrector.remove(send_node.loc.dot)
corrector.remove(block_argument_range(send_node)) unless node.numblock_type?
end

def redundant_receiver?(send_nodes, node)
proc = if node.numblock_type?
->(n) { n.receiver.lvar_type? && n.receiver.source == '_1' }
else
arg = node.arguments.first
->(n) { same_value?(arg, n.receiver) }
end

send_nodes.all?(&proc)
end

def block_argument_range(node)
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails/reversible_migration.rb
Expand Up @@ -229,6 +229,8 @@ def on_block(node)
check_change_table_node(node.send_node, node.body)
end

alias on_numblock on_block

private

def check_irreversible_schema_statement_node(node)
Expand Down
30 changes: 30 additions & 0 deletions spec/rubocop/cop/rails/redundant_receiver_in_with_options_spec.rb
Expand Up @@ -29,6 +29,36 @@ class Account < ApplicationRecord
RUBY
end

context 'Ruby >= 2.7', :ruby27 do
it 'registers an offense and corrects using explicit receiver in `with_options`' do
expect_offense(<<~RUBY)
class Account < ApplicationRecord
with_options dependent: :destroy do
_1.has_many :customers
^^ Redundant receiver in `with_options`.
_1.has_many :products
^^ Redundant receiver in `with_options`.
_1.has_many :invoices
^^ Redundant receiver in `with_options`.
_1.has_many :expenses
^^ Redundant receiver in `with_options`.
end
end
RUBY

expect_correction(<<~RUBY)
class Account < ApplicationRecord
with_options dependent: :destroy do
has_many :customers
has_many :products
has_many :invoices
has_many :expenses
end
end
RUBY
end
end

it 'does not register an offense when using implicit receiver in `with_options`' do
expect_no_offenses(<<~RUBY)
class Account < ApplicationRecord
Expand Down
16 changes: 16 additions & 0 deletions spec/rubocop/cop/rails/reversible_migration_spec.rb
Expand Up @@ -35,6 +35,14 @@ def change
end
RUBY

context 'Ruby >= 2.7', :ruby27 do
it_behaves_like 'accepts', 'create_table using numbered parameter', <<~RUBY
create_table :users do
_1.string :name
end
RUBY
end

it_behaves_like 'offense', 'execute', <<~RUBY
execute "ALTER TABLE `pages_linked_pages` ADD UNIQUE `page_id_linked_page_id` (`page_id`,`linked_page_id`)"
RUBY
Expand Down Expand Up @@ -227,6 +235,14 @@ def change
end
RUBY

context 'Ruby >= 2.7', :ruby27 do
it_behaves_like 'offense', 'change_table(with change_default)', <<~RUBY
change_table :users do
_1.change_default :authorized, 1
end
RUBY
end

context 'remove' do
context 'Rails >= 6.1', :rails61 do
it_behaves_like 'accepts', 't.remove (with type)', <<~RUBY
Expand Down