diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b38d2d5cf..517216a7d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +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][]) * [#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][]) @@ -353,3 +354,4 @@ [@ohbarye]: https://github.com/ohbarye [@tubaxenor]: https://github.com/tubaxenor [@olivierbuffon]: https://github.com/olivierbuffon +[@leonp1991]: https://github.com/leonp1991 diff --git a/config/default.yml b/config/default.yml index 139dd21743..2f1727ffbd 100644 --- a/config/default.yml +++ b/config/default.yml @@ -362,6 +362,11 @@ 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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index fabcdae60d..3a598f5481 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -53,6 +53,7 @@ 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] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 8334dff47e..21f37741ee 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -1825,6 +1825,68 @@ 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 |=== diff --git a/lib/rubocop/cop/rails/implemented_migration.rb b/lib/rubocop/cop/rails/implemented_migration.rb new file mode 100644 index 0000000000..7d0a4d8076 --- /dev/null +++ b/lib/rubocop/cop/rails/implemented_migration.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop checks whether the migration implements + # either a `change` method or both an `up` and a `down` + # method. + # + # @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 + # + # # <----- 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 + class ImplementedMigration < Base + MSG = 'Migrations must contain either a change method, or ' \ + 'both an up and a down method.' + + def_node_matcher :migration_class?, <<~PATTERN + (class + (const nil? _) + (send + (const (const nil? :ActiveRecord) :Migration) + :[] + (float _)) + _) + PATTERN + + def_node_matcher :change_method?, <<~PATTERN + [ #migration_class? `(def :change (args) _) ] + PATTERN + + def_node_matcher :up_and_down_methods?, <<~PATTERN + [ #migration_class? `(def :up (args) _) `(def :down (args) _) ] + PATTERN + + def on_class(node) + return if change_method?(node) + return if up_and_down_methods?(node) + + add_offense(node) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 3c602d9182..62efe7983e 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -42,6 +42,7 @@ 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' diff --git a/spec/rubocop/cop/rails/implemented_migration_spec.rb b/spec/rubocop/cop/rails/implemented_migration_spec.rb new file mode 100644 index 0000000000..855b23a606 --- /dev/null +++ b/spec/rubocop/cop/rails/implemented_migration_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::ImplementedMigration, :config do + it 'does not register an offense with a change method' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def change + add_column :users, :email, :text, null: false + end + end + RUBY + end + + 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. + + def up + add_column :users, :email, :text, null: false + end + end + RUBY + end + + 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. + + def down + remove_column :users, :email + end + end + RUBY + end + + it 'does not register an offense with an up and a down method' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def up + add_column :users, :email, :text, null: false + end + + def down + remove_column :users, :email + end + end + RUBY + end + + 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. + + def chance + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'does not register an offense with helper methods' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[6.0] + def change + add_users_column :email, :text, null: false + end + + private + + def add_users_column(column_name, null: false) + add_column :users, column_name, type, null: null + end + end + RUBY + end + + 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. + + def chance + add_column :users, :email, :text, null: false + end + end + RUBY + end + + it 'does not register offenses correctly with any migration class' do + expect_no_offenses(<<~RUBY) + class SomeMigration < ActiveRecord::Migration[5.2] + def change + add_column :users, :email, :text, null: false + 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