(fix) koic's suggestions

CHANGELOG.md

@@ -4,7 +4,7 @@
 
 ### New features
 
-* [#457](https://github.com/rubocop/rubocop-rails/pull/457): New cop `Rails/ImplementedMigration` checks for migrations that have a typo in a method name or are half implemented. ([@leonp1991][])
+* [#457](https://github.com/rubocop/rubocop-rails/pull/457): Add new `Rails/ReversibleMigrationMethodDefinition` cop. ([@leonp1991][])
 * [#446](https://github.com/rubocop/rubocop-rails/issues/446): Add new `Rails/RequireDependency` cop. ([@tubaxenor][])
 * [#458](https://github.com/rubocop/rubocop-rails/issues/458): Add new `Rails/TimeZoneAssignment` cop. ([@olivierbuffon][])
 

config/default.yml

@@ -362,11 +362,6 @@ Rails/IgnoredSkipActionFilterOption:
  Include:
  - app/controllers/**/*.rb
 
-Rails/ImplementedMigration:
- Description: 'Checks for migrations that have a typo in a method name or are half implemented.'
- Enabled: false
- VersionAdded: '2.10'
-
 Rails/IndexBy:
  Description: 'Prefer `index_by` over `each_with_object`, `to_h`, or `map`.'
  Enabled: true
@@ -625,6 +620,13 @@ Rails/ReversibleMigration:
  Include:
  - db/migrate/*.rb
 
+Rails/ReversibleMigrationMethodDefinition:
+ Description: 'Checks whether the migration implements either a `change` method or both an `up` and a `down` method.'
+ Enabled: false
+ VersionAdded: '2.10'
+ include:
+ - db/migrate/*.rb
+
 Rails/SafeNavigation:
  Description: "Use Ruby's safe navigation operator (`&.`) instead of `try!`."
  Enabled: true

docs/modules/ROOT/pages/cops.adoc

@@ -53,7 +53,6 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 * xref:cops_rails.adoc#railshttppositionalarguments[Rails/HttpPositionalArguments]
 * xref:cops_rails.adoc#railshttpstatus[Rails/HttpStatus]
 * xref:cops_rails.adoc#railsignoredskipactionfilteroption[Rails/IgnoredSkipActionFilterOption]
-* xref:cops_rails.adoc#railsimplementedmigration[Rails/ImplementedMigration]
 * xref:cops_rails.adoc#railsindexby[Rails/IndexBy]
 * xref:cops_rails.adoc#railsindexwith[Rails/IndexWith]
 * xref:cops_rails.adoc#railsinquiry[Rails/Inquiry]
@@ -87,6 +86,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 * xref:cops_rails.adoc#railsrequestreferer[Rails/RequestReferer]
 * xref:cops_rails.adoc#railsrequiredependency[Rails/RequireDependency]
 * xref:cops_rails.adoc#railsreversiblemigration[Rails/ReversibleMigration]
+* xref:cops_rails.adoc#railsreversiblemigrationmethoddefinition[Rails/ReversibleMigrationMethodDefinition]
 * xref:cops_rails.adoc#railssafenavigation[Rails/SafeNavigation]
 * xref:cops_rails.adoc#railssafenavigationwithblank[Rails/SafeNavigationWithBlank]
 * xref:cops_rails.adoc#railssavebang[Rails/SaveBang]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -1825,68 +1825,6 @@ end
 
 * https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options
 
-== Rails/ImplementedMigration
-
-|===
-| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
-
-| Disabled
-| Yes
-| No
-| 2.10
-| -
-|===
-
-This cop checks whether the migration implements
-either a `change` method or both an `up` and a `down`
-method.
-
-=== Examples
-
-[source,ruby]
-----
-# bad
-class SomeMigration < ActiveRecord::Migration[6.0]
- def chance # <----- typo
- # migration
- end
-end
-
-class SomeMigration < ActiveRecord::Migration[6.0]
- def up
- # up migration
- end
-
- # <----- missing down method
-end
-
-class SomeMigration < ActiveRecord::Migration[6.0]
- # <----- missing up method
-
- def down
- # down migration
- end
-end
-
-# good
-class SomeMigration < ActiveRecord::Migration[6.0]
- def change
- # migration
- end
-end
-
-# good
-class SomeMigration < ActiveRecord::Migration[6.0]
- def up
- # up migration
- end
-
- def down
- # down migration
- end
-end
-----
-
 == Rails/IndexBy
 
 |===
@@ -3756,6 +3694,72 @@ end
 * https://rails.rubystyle.guide#reversible-migration
 * https://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html
 
+== Rails/ReversibleMigrationMethodDefinition
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Disabled
+| Yes
+| No
+| 2.10
+| -
+|===
+
+This cop checks whether the migration implements
+either a `change` method or both an `up` and a `down`
+method.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+class SomeMigration < ActiveRecord::Migration[6.0]
+ def up
+ # up migration
+ end
+
+ # <----- missing down method
+end
+
+class SomeMigration < ActiveRecord::Migration[6.0]
+ # <----- missing up method
+
+ def down
+ # down migration
+ end
+end
+
+# good
+class SomeMigration < ActiveRecord::Migration[6.0]
+ def change
+ # reversible migration
+ end
+end
+
+# good
+class SomeMigration < ActiveRecord::Migration[6.0]
+ def up
+ # up migration
+ end
+
+ def down
+ # down migration
+ end
+end
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| include
+| `db/migrate/*.rb`
+| Array
+|===
+
 == Rails/SafeNavigation
 
 |===

lib/rubocop/cop/rails/implemented_migration.rb → lib/rubocop/cop/rails/reversible_migration_method_definition.rb

@@ -10,12 +10,6 @@ module Rails
  # @example
  # # bad
  # class SomeMigration < ActiveRecord::Migration[6.0]
- # def chance # <----- typo
- # # migration
- # end
- # end
- #
- # class SomeMigration < ActiveRecord::Migration[6.0]
  # def up
  # # up migration
  # end
@@ -34,7 +28,7 @@ module Rails
  # # good
  # class SomeMigration < ActiveRecord::Migration[6.0]
  # def change
- # # migration
+ # # reversible migration
  # end
  # end
  #
@@ -48,9 +42,9 @@ module Rails
  # # down migration
  # end
  # end
- class ImplementedMigration < Base
- MSG = 'Migrations must contain either a change method, or ' \
- 'both an up and a down method.'
+ class ReversibleMigrationMethodDefinition < Base
+ MSG = 'Migrations must contain either a `change` method, or ' \
+ 'both an `up` and a `down` method.'
 
  def_node_matcher :migration_class?, <<~PATTERN
  (class
@@ -71,8 +65,7 @@ class ImplementedMigration < Base
  PATTERN
 
  def on_class(node)
- return if change_method?(node)
- return if up_and_down_methods?(node)
+ return if change_method?(node) || up_and_down_methods?(node)
 
  add_offense(node)
  end

lib/rubocop/cop/rails_cops.rb

@@ -42,7 +42,6 @@
 require_relative 'rails/http_positional_arguments'
 require_relative 'rails/http_status'
 require_relative 'rails/ignored_skip_action_filter_option'
-require_relative 'rails/implemented_migration'
 require_relative 'rails/index_by'
 require_relative 'rails/index_with'
 require_relative 'rails/inquiry'
@@ -76,6 +75,7 @@
 require_relative 'rails/request_referer'
 require_relative 'rails/require_dependency'
 require_relative 'rails/reversible_migration'
+require_relative 'rails/reversible_migration_method_definition'
 require_relative 'rails/safe_navigation'
 require_relative 'rails/safe_navigation_with_blank'
 require_relative 'rails/save_bang'

spec/rubocop/cop/rails/implemented_migration_spec.rb → spec/rubocop/cop/rails/reversible_migration_method_definition_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Rails::ImplementedMigration, :config do
+RSpec.describe RuboCop::Cop::Rails::ReversibleMigrationMethodDefinition, :config do
  it 'does not register an offense with a change method' do
  expect_no_offenses(<<~RUBY)
  class SomeMigration < ActiveRecord::Migration[6.0]
@@ -14,7 +14,7 @@ def change
  it 'registers an offense with only an up method' do
  expect_offense(<<~RUBY)
  class SomeMigration < ActiveRecord::Migration[6.0]
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method.
 
  def up
  add_column :users, :email, :text, null: false
@@ -26,7 +26,7 @@ def up
  it 'registers an offense with only a down method' do
  expect_offense(<<~RUBY)
  class SomeMigration < ActiveRecord::Migration[6.0]
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method.
 
  def down
  remove_column :users, :email
@@ -49,11 +49,10 @@ def down
  RUBY
  end
 
- it 'registers an offense with a typo\'d change method' do
+ it "registers an offense with a typo'd change method" do
  expect_offense(<<~RUBY)
  class SomeMigration < ActiveRecord::Migration[6.0]
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
-
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method.
  def chance
  add_column :users, :email, :text, null: false
  end
@@ -79,19 +78,8 @@ def add_users_column(column_name, null: false)
 
  it 'registers offenses correctly with any migration class' do
  expect_offense(<<~RUBY)
- class SomeMigration < ActiveRecord::Migration[5.1]
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
-
- def chance
- add_column :users, :email, :text, null: false
- end
- end
- RUBY
-
- expect_offense(<<~RUBY)
- class SomeMigration < ActiveRecord::Migration[7.1]
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a change method, or both an up and a down method.
-
+ class SomeMigration < ActiveRecord::Migration[5.2]
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Migrations must contain either a `change` method, or both an `up` and a `down` method.
  def chance
  add_column :users, :email, :text, null: false
  end
@@ -107,13 +95,5 @@ def change
  end
  end
  RUBY
-
- expect_no_offenses(<<~RUBY)
- class SomeMigration < ActiveRecord::Migration[7.1]
- def change
- add_column :users, :email, :text, null: false
- end
- end
- RUBY
  end
 end