Skip to content

Commit

Permalink
[Fix rubocop#1410] Make registered cops aware of `AllCops: MigratedSc…
Browse files Browse the repository at this point in the history
…hemaVersion`

When implementing rubocop#1383, the detection of `AllCops: MigratedSchemaVersion` was
initially considered only for cops related to migrations.

However, feedback received later, such as in rubocop#1410, indicated that it is also expected to apply to
`Style`, `Lint`, and other categories. This suggestion is reasonable, as warnings for migrated files
may not be limited to database columns but could also include Ruby programming logic.
Excluding `Style` and `Lint` from this consideration would not align with this feedback.

This PR modifies the behavior so that all registered cops can detect the value of `AllCops: MigratedSchemaVersion`.

Fixes rubocop#1410.
  • Loading branch information
koic committed Jan 14, 2025
1 parent b1fbd49 commit cc7b502
Show file tree
Hide file tree
Showing 14 changed files with 79 additions and 38 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1410](https://github.com/rubocop/rubocop-rails/issues/1410): Make registered cops aware of `AllCops: MigratedSchemaVersion`. ([@koic][])
3 changes: 3 additions & 0 deletions lib/rubocop-rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@

require_relative 'rubocop/cop/rails_cops'

require_relative 'rubocop/rails/migration_file_skippable'
RuboCop::Rails::MigrationFileSkippable.apply_to_cops!

RuboCop::Cop::Style::HashExcept.minimum_target_ruby_version(2.0)

RuboCop::Cop::Style::InverseMethods.singleton_class.prepend(
Expand Down
29 changes: 0 additions & 29 deletions lib/rubocop/cop/mixin/migrations_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,35 +21,6 @@ def in_migration?(node)
migration_class?(class_node)
end
end

# rubocop:disable Style/DocumentDynamicEvalDefinition
%i[on_send on_csend on_block on_numblock on_class].each do |method|
class_eval(<<~RUBY, __FILE__, __LINE__ + 1)
def #{method}(node)
return if already_migrated_file?
super if method(__method__).super_method
end
RUBY
end
# rubocop:enable Style/DocumentDynamicEvalDefinition

private

def already_migrated_file?
return false unless migrated_schema_version

match_data = File.basename(processed_source.file_path).match(/(?<timestamp>\d{14})/)
schema_version = match_data['timestamp'] if match_data

return false unless schema_version

schema_version <= migrated_schema_version.to_s # Ignore applied migration files.
end

def migrated_schema_version
config.for_all_cops.fetch('MigratedSchemaVersion', nil)
end
end
end
end
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/add_column_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module Rails
class AddColumnIndex < Base
extend AutoCorrector
include RangeHelp
prepend MigrationsHelper
include MigrationsHelper

MSG = '`add_column` does not accept an `index` key, use `add_index` instead.'
RESTRICT_ON_SEND = %i[add_column].freeze
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/bulk_change_table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ module Rails
# end
class BulkChangeTable < Base
include DatabaseTypeResolvable
prepend MigrationsHelper
include MigrationsHelper

MSG_FOR_CHANGE_TABLE = <<~MSG.chomp
You can combine alter queries using `bulk: true` options.
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/dangerous_column_names.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Rails
# # good
# add_column :users, :saved
class DangerousColumnNames < Base # rubocop:disable Metrics/ClassLength
prepend MigrationsHelper
include MigrationsHelper

COLUMN_TYPE_METHOD_NAMES = %i[
bigint
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/migration_class_name.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module Rails
#
class MigrationClassName < Base
extend AutoCorrector
prepend MigrationsHelper
include MigrationsHelper

MSG = 'Replace with `%<camelized_basename>s` that matches the file name.'

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/not_null_column.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module Rails
# change_column_null :products, :category_id, false
class NotNullColumn < Base
include DatabaseTypeResolvable
prepend MigrationsHelper
include MigrationsHelper

MSG = 'Do not add a NOT NULL column without a default value.'
RESTRICT_ON_SEND = %i[add_column add_reference].freeze
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/reversible_migration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ module Rails
# remove_index :users, column: :email
# end
class ReversibleMigration < Base
prepend MigrationsHelper
include MigrationsHelper

MSG = '%<action>s is not reversible.'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ module Rails
# end
# end
class ReversibleMigrationMethodDefinition < Base
prepend MigrationsHelper
include MigrationsHelper

MSG = 'Migrations must contain either a `change` method, or both an `up` and a `down` method.'

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/schema_comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module Rails
#
class SchemaComment < Base
include ActiveRecordMigrationsHelper
prepend MigrationsHelper
include MigrationsHelper

COLUMN_MSG = 'New database column without `comment`.'
TABLE_MSG = 'New database table without `comment`.'
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/three_state_boolean_column.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module Rails
# t.boolean :active, default: true, null: false
#
class ThreeStateBooleanColumn < Base
prepend MigrationsHelper
include MigrationsHelper

MSG = 'Boolean columns should always have a default value and a `NOT NULL` constraint.'
RESTRICT_ON_SEND = %i[add_column column boolean].freeze
Expand Down
54 changes: 54 additions & 0 deletions lib/rubocop/rails/migration_file_skippable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# frozen_string_literal: true

module RuboCop
module Rails
# This module allows cops to detect and ignore files that have already been migrated
# by leveraging the `AllCops: MigratedSchemaVersion` configuration.
#
# [source,yaml]
# -----
# AllCops:
# MigratedSchemaVersion: '20241225000000'
# -----
#
# When applied to cops, it dynamically overrides the `on_*` methods for all supported
# node types, ensuring that cops skip processing if the file is determined to be a
# migrated file based on the schema version.
module MigrationFileSkippable
Parser::Meta::NODE_TYPES.each do |node_type|
method_name = :"on_#{node_type}"

class_eval(<<~RUBY, __FILE__, __LINE__ + 1)
def #{method_name}(node, *args) # def on_if(node, *args)
return if already_migrated_file? # return if already_migrated_file?
#
super if method(__method__).super_method # super if method(__method__).super_method
rescue NameError # rescue NameError
# noop # # noop
end # end
RUBY
end

def self.apply_to_cops!
RuboCop::Cop::Registry.all.each { |cop| cop.prepend(MigrationFileSkippable) }
end

private

def already_migrated_file?
return false unless migrated_schema_version

match_data = File.basename(processed_source.file_path).match(/(?<timestamp>\d{14})/)
schema_version = match_data['timestamp'] if match_data

return false unless schema_version

schema_version <= migrated_schema_version.to_s # Ignore applied migration files.
end

def migrated_schema_version
config.for_all_cops.fetch('MigratedSchemaVersion', nil)
end
end
end
end
12 changes: 12 additions & 0 deletions spec/rubocop/cop/rails/presence_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -441,4 +441,16 @@
end
RUBY
end

context 'when inspection file that have already been migrated' do
let(:config) do
RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => '20240101010101' })
end

it 'does not register an offense and corrects when `a.present? ? a : nil`' do
expect_no_offenses(<<~RUBY, '20190101010101_add_column_to_table.rb')
a.present? ? a : nil
RUBY
end
end
end

0 comments on commit cc7b502

Please sign in to comment.