Merge pull request rubocop#547 from mollerhoj/action-order-cop

Action Order Cop

Author: koic

changelog/new_action_order_cop.md

@@ -0,0 +1 @@
+* [#547](https://github.com/rubocop/rubocop/pull/547): action order cop. ([@mollerhoj][])

config/default.yml

@@ -95,6 +95,21 @@ Rails/ActionFilter:
     - app/controllers/**/*.rb
     - app/mailers/**/*.rb
 
+Rails/ActionOrder:
+  Description: 'Enforce consistent ordering of controller actions.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+  ExpectedOrder:
+    - index
+    - show
+    - new
+    - edit
+    - create
+    - update
+    - destroy
+  Include:
+    - app/controllers/**/*.rb
+
 Rails/ActiveRecordAliases:
   Description: >-
                   Avoid Active Record aliases:

docs/modules/ROOT/pages/cops.adoc

@@ -19,6 +19,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 * xref:cops_rails.adoc#railsactioncontrollerflashbeforerender[Rails/ActionControllerFlashBeforeRender]
 * xref:cops_rails.adoc#railsactioncontrollertestcase[Rails/ActionControllerTestCase]
 * xref:cops_rails.adoc#railsactionfilter[Rails/ActionFilter]
+* xref:cops_rails.adoc#railsactionorder[Rails/ActionOrder]
 * xref:cops_rails.adoc#railsactiverecordaliases[Rails/ActiveRecordAliases]
 * xref:cops_rails.adoc#railsactiverecordcallbacksorder[Rails/ActiveRecordCallbacksOrder]
 * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -172,6 +172,66 @@ skip_after_filter :do_stuff
 | Array
 |===
 
+== Rails/ActionOrder
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Disabled
+| Yes
+| Yes
+| 2.13
+| -
+|===
+
+This cop enforces consistent ordering of the standard Rails RESTful
+controller actions.
+
+The cop is configurable and can enforce any ordering of the standard
+actions. All other methods are ignored.
+
+[source,yaml]
+----
+ Rails/ActionOrder:
+   ExpectedOrder:
+     - index
+     - show
+     - new
+     - edit
+     - create
+     - update
+     - destroy
+----
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+def index; end
+def destroy; end
+def show; end
+
+# good
+def index; end
+def show; end
+def destroy; end
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| ExpectedOrder
+| `index`, `show`, `new`, `edit`, `create`, `update`, `destroy`
+| Array
+
+| Include
+| `app/controllers/**/*.rb`
+| Array
+|===
+
 == Rails/ActiveRecordAliases
 
 |===

lib/rubocop/cop/rails/action_order.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop enforces consistent ordering of the standard Rails RESTful
+      # controller actions.
+      #
+      # The cop is configurable and can enforce any ordering of the standard
+      # actions. All other methods are ignored.
+      #
+      # [source,yaml]
+      # ----
+      #  Rails/ActionOrder:
+      #    ExpectedOrder:
+      #      - index
+      #      - show
+      #      - new
+      #      - edit
+      #      - create
+      #      - update
+      #      - destroy
+      # ----
+      #
+      # @example
+      #   # bad
+      #   def index; end
+      #   def destroy; end
+      #   def show; end
+      #
+      #   # good
+      #   def index; end
+      #   def show; end
+      #   def destroy; end
+      class ActionOrder < Base
+        extend AutoCorrector
+        include VisibilityHelp
+        include DefNode
+
+        MSG = 'Action `%<current>s` should appear before `%<previous>s`.'
+
+        def_node_search :action_declarations, '(def {%1} ...)'
+
+        def on_class(node)
+          action_declarations(node, actions).each_cons(2) do |previous, current|
+            next if node_visibility(current) != :public || non_public?(current)
+            next if find_index(current) >= find_index(previous)
+
+            register_offense(previous, current)
+          end
+        end
+
+        private
+
+        def expected_order
+          cop_config['ExpectedOrder'].map(&:to_sym)
+        end
+
+        def actions
+          @actions ||= Set.new(expected_order)
+        end
+
+        def find_index(node)
+          expected_order.find_index(node.method_name)
+        end
+
+        def register_offense(previous, current)
+          message = format(
+            MSG,
+            expected_order: expected_order.join(', '),
+            previous: previous.method_name,
+            current: current.method_name
+          )
+          add_offense(current, message: message) do |corrector|
+            corrector.replace(current, previous.source)
+            corrector.replace(previous, current.source)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -11,6 +11,7 @@
 require_relative 'rails/action_controller_flash_before_render'
 require_relative 'rails/action_controller_test_case'
 require_relative 'rails/action_filter'
+require_relative 'rails/action_order'
 require_relative 'rails/active_record_aliases'
 require_relative 'rails/active_record_callbacks_order'
 require_relative 'rails/active_record_override'

spec/rubocop/cop/rails/action_order_spec.rb

@@ -0,0 +1,159 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::ActionOrder, :config do
+  it 'detects unconventional order of actions' do
+    expect_offense(<<~RUBY)
+      class UserController < ApplicationController
+        def show; end
+        def index; end
+        ^^^^^^^^^^^^^^ Action `index` should appear before `show`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class UserController < ApplicationController
+        def index; end
+        def show; end
+      end
+    RUBY
+  end
+
+  it 'supports methods with content' do
+    expect_offense(<<~RUBY)
+      class UserController < ApplicationController
+        def show
+          @user = User.find(params[:id])
+        end
+
+        def index; end
+        ^^^^^^^^^^^^^^ Action `index` should appear before `show`.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class UserController < ApplicationController
+        def index; end
+
+        def show
+          @user = User.find(params[:id])
+        end
+      end
+    RUBY
+  end
+
+  it 'respects order of duplicate methods' do
+    expect_offense(<<~RUBY)
+      class UserController < ApplicationController
+        def edit; end
+        def index # first
+        ^^^^^^^^^^^^^^^^^ Action `index` should appear before `edit`.
+        end
+        def show; end
+        def index # second
+        ^^^^^^^^^^^^^^^^^^ Action `index` should appear before `show`.
+        end
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      class UserController < ApplicationController
+        def index # first
+        end
+        def index # second
+        end
+        def show; end
+        def edit; end
+      end
+    RUBY
+  end
+
+  it 'ignores non standard controller actions' do
+    expect_no_offenses(<<~RUBY)
+      class UserController < ApplicationController
+        def index; end
+        def commit; end
+        def show; end
+      end
+    RUBY
+  end
+
+  it 'does not touch protected actions' do
+    expect_no_offenses(<<~RUBY)
+      class UserController < ApplicationController
+        def show; end
+        protected
+        def index; end
+      end
+    RUBY
+  end
+
+  it 'does not touch inline protected actions' do
+    expect_no_offenses(<<~RUBY)
+      class UserController < ApplicationController
+        def show; end
+        protected def index; end
+      end
+    RUBY
+  end
+
+  it 'does not touch private actions' do
+    expect_no_offenses(<<~RUBY)
+      class UserController < ApplicationController
+        def show; end
+        private
+        def index; end
+      end
+    RUBY
+  end
+
+  it 'does not touch inline private actions' do
+    expect_no_offenses(<<~RUBY)
+      class UserController < ApplicationController
+        def show; end
+        private def index; end
+      end
+    RUBY
+  end
+
+  context 'with custom ordering' do
+    it 'enforces custom order' do
+      cop_config['ExpectedOrder'] = %w[show index new edit create update destroy]
+
+      expect_offense(<<~RUBY)
+        class UserController < ApplicationController
+          def index; end
+          def show; end
+          ^^^^^^^^^^^^^ Action `show` should appear before `index`.
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class UserController < ApplicationController
+          def show; end
+          def index; end
+        end
+      RUBY
+    end
+
+    it 'does not require all actions to be specified' do
+      cop_config['ExpectedOrder'] = %w[show index]
+
+      expect_offense(<<~RUBY)
+        class UserController < ApplicationController
+          def index; end
+          def edit; end
+          def show; end
+          ^^^^^^^^^^^^^ Action `show` should appear before `index`.
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        class UserController < ApplicationController
+          def show; end
+          def edit; end
+          def index; end
+        end
+      RUBY
+    end
+  end
+end