From effdb0a6b0cbbc8977cd79b952776036d50fc878 Mon Sep 17 00:00:00 2001 From: mollerhoj Date: Mon, 13 Sep 2021 18:29:21 +0200 Subject: [PATCH] action order cop --- changelog/new_action_order_cop.md | 1 + config/default.yml | 15 ++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rails.adoc | 60 ++++++++ lib/rubocop/cop/rails/action_order.rb | 82 ++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/action_order_spec.rb | 159 ++++++++++++++++++++ 7 files changed, 319 insertions(+) create mode 100644 changelog/new_action_order_cop.md create mode 100644 lib/rubocop/cop/rails/action_order.rb create mode 100644 spec/rubocop/cop/rails/action_order_spec.rb diff --git a/changelog/new_action_order_cop.md b/changelog/new_action_order_cop.md new file mode 100644 index 0000000000..65fb2a56d5 --- /dev/null +++ b/changelog/new_action_order_cop.md @@ -0,0 +1 @@ +* [#547](https://github.com/rubocop/rubocop/pull/547): action order cop. ([@mollerhoj][]) diff --git a/config/default.yml b/config/default.yml index 6676ca47b0..c1eefc5a37 100644 --- a/config/default.yml +++ b/config/default.yml @@ -49,6 +49,21 @@ Rails/ActionFilter: Include: - app/controllers/**/*.rb +Rails/ActionOrder: + Description: 'Enforce consistent ordering of controller actions.' + Enabled: pending + VersionAdded: '<>' + ExpectedOrder: + - index + - show + - new + - edit + - create + - update + - destroy + Include: + - app/controllers/**/*.rb + Rails/ActiveRecordAliases: Description: >- Avoid Active Record aliases: diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index cb51e38bcb..023d023e49 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -17,6 +17,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide]. === Department xref:cops_rails.adoc[Rails] * 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] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index ea2469b2a8..cc028df52a 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -63,6 +63,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 |=== diff --git a/lib/rubocop/cop/rails/action_order.rb b/lib/rubocop/cop/rails/action_order.rb new file mode 100644 index 0000000000..45d9616521 --- /dev/null +++ b/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 `%s` should appear before `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 7c00103b00..358aeea0a5 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -6,6 +6,7 @@ require_relative 'mixin/target_rails_version' 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' diff --git a/spec/rubocop/cop/rails/action_order_spec.rb b/spec/rubocop/cop/rails/action_order_spec.rb new file mode 100644 index 0000000000..3b5bb1b68c --- /dev/null +++ b/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