diff --git a/CHANGELOG.md b/CHANGELOG.md index 67c0084c94..5fe718175a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#123](https://github.com/rubocop-hq/rubocop-rails/pull/123): Add new `Rails/ApplicationController` and `Rails/ApplicationMailer` cops. ([@eugeneius][]) +* [#139](https://github.com/rubocop-hq/rubocop-rails/pull/139): Add new `Rails/RescueFromExceptionsVariableName` cop. ([@anthony-robin][]) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index 9b2b62de8d..e9a223362f 100644 --- a/config/default.yml +++ b/config/default.yml @@ -386,6 +386,12 @@ Rails/RequestReferer: - referer - referrer +Rails/RescueFromExceptionsVariableName: + Description: 'Use consistent rescued exceptions variables naming.' + Enabled: true + VersionAdded: '2.4' + PreferredName: e + Rails/ReversibleMigration: Description: 'Checks whether the change method of the migration file is reversible.' StyleGuide: 'https://rails.rubystyle.guide#reversible-migration' diff --git a/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb b/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb new file mode 100644 index 0000000000..0760e0d6ec --- /dev/null +++ b/lib/rubocop/cop/rails/rescue_from_exceptions_variable_name.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop makes sure that rescued exceptions variables are named as + # expected. + # + # The `PreferredName` config option takes a `String`. It represents + # the required name of the variable. Its default is `e` that is read + # from `Naming/RescuedExceptionsVariableName` cop in the main Rubocop + # repository. + # + # @example PreferredName: e (default) + # # bad + # rescue_from MyException do |exception| + # # do something + # end + # + # # good + # rescue_from MyException do |e| + # # do something + # end + # + # # good + # rescue_from MyException do |_e| + # # do something + # end + # + # @example PreferredName: exception + # # bad + # rescue_from MyException do |e| + # # do something + # end + # + # # good + # rescue_from MyException do |exception| + # # do something + # end + # + # # good + # rescue_from MyException do |_exception| + # # do something + # end + # + class RescueFromExceptionsVariableName < Cop + include ConfigurableEnforcedStyle + + MSG = 'Use `%s` instead of `%s`.' + + def on_block(node) + name = variable_name(node) + return unless name + return if preferred_name(name).to_sym == name + + add_offense(node, location: offense_range(node)) + end + + def autocorrect(node) + lambda do |corrector| + offending_name = variable_name(node) + preferred_name = preferred_name(offending_name) + corrector.replace(offense_range(node), "|#{preferred_name}|") + end + end + + private + + def offense_range(resbody) + variable = resbody.node_parts[1] + variable.loc.expression + end + + def preferred_name(variable_name) + preferred_name = cop_config.fetch('PreferredName', 'e') + if variable_name.to_s.start_with?('_') + "_#{preferred_name}" + else + preferred_name + end + end + + def variable_name(node) + asgn_node = node.node_parts[1] + return unless asgn_node + + asgn_node.children.last&.source&.to_sym + end + + def message(node) + offending_name = variable_name(node) + preferred_name = preferred_name(offending_name) + format(MSG, preferred: preferred_name, bad: offending_name) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 0f5d167883..19c8086b22 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -48,6 +48,7 @@ require_relative 'rails/refute_methods' require_relative 'rails/relative_date_constant' require_relative 'rails/request_referer' +require_relative 'rails/rescue_from_exceptions_variable_name' require_relative 'rails/reversible_migration' require_relative 'rails/safe_navigation' require_relative 'rails/save_bang' diff --git a/manual/cops.md b/manual/cops.md index 661364c625..5077243abb 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -47,6 +47,7 @@ * [Rails/RefuteMethods](cops_rails.md#railsrefutemethods) * [Rails/RelativeDateConstant](cops_rails.md#railsrelativedateconstant) * [Rails/RequestReferer](cops_rails.md#railsrequestreferer) +* [Rails/RescueFromExceptionsVariableName](cops_rails.md#railsrescuefromexceptionsvariablename) * [Rails/ReversibleMigration](cops_rails.md#railsreversiblemigration) * [Rails/SafeNavigation](cops_rails.md#railssafenavigation) * [Rails/SaveBang](cops_rails.md#railssavebang) diff --git a/manual/cops_rails.md b/manual/cops_rails.md index 60c061078c..b37ee356a3 100644 --- a/manual/cops_rails.md +++ b/manual/cops_rails.md @@ -1913,6 +1913,65 @@ Name | Default value | Configurable values --- | --- | --- EnforcedStyle | `referer` | `referer`, `referrer` +## Rails/RescueFromExceptionsVariableName + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 2.4 | - + +This cop makes sure that rescued exceptions variables are named as +expected. + +The `PreferredName` config option takes a `String`. It represents +the required name of the variable. Its default is `e` that is read +from `Naming/RescuedExceptionsVariableName` cop in the main Rubocop +repository. + +### Examples + +#### PreferredName: e (default) + +```ruby +# bad +rescue_from MyException do |exception| + # do something +end + +# good +rescue_from MyException do |e| + # do something +end + +# good +rescue_from MyException do |_e| + # do something +end +``` +#### PreferredName: exception + +```ruby +# bad +rescue_from MyException do |e| + # do something +end + +# good +rescue_from MyException do |exception| + # do something +end + +# good +rescue_from MyException do |_exception| + # do something +end +``` + +### Configurable attributes + +Name | Default value | Configurable values +--- | --- | --- +PreferredName | `e` | String + ## Rails/ReversibleMigration Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb b/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb new file mode 100644 index 0000000000..8082ab4b9e --- /dev/null +++ b/spec/rubocop/cop/rails/rescue_from_exceptions_variable_name_spec.rb @@ -0,0 +1,166 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RescueFromExceptionsVariableName, :config do + subject(:cop) { described_class.new(config) } + + it 'does not register an offense without variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do + # do something + end + RUBY + end + + context 'with default config' do + context 'with default variable' do + it 'does not register an offense with a single rescued exception' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |e| + # do something + end + RUBY + end + + it 'does not register an offense with multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException do |e| + # do something + end + RUBY + end + + it 'does not register an offense with underscored prefix variable' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |_e| + # do something + end + RUBY + end + + it 'does not register an offense using splat operator' do + expect_no_offenses(<<~RUBY) + rescue_from *handled do |e| + # do something + end + RUBY + end + end + + context 'when using another variable' do + it 'registers an offense with a single rescued exception' do + expect_offense(<<~RUBY) + rescue_from MyException do |exception| + ^^^^^^^^^^^ Use `e` instead of `exception`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |e| + # do something + end + RUBY + end + + it 'registers an offense with multiple rescued exceptions' do + expect_offense(<<~RUBY) + rescue_from MyException, MyOtherException do |exception| + ^^^^^^^^^^^ Use `e` instead of `exception`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException, MyOtherException do |e| + # do something + end + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException do |_exception| + ^^^^^^^^^^^^ Use `_e` instead of `_exception`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |_e| + # do something + end + RUBY + end + + it 'registers an offense using splat operator' do + expect_offense(<<~RUBY) + rescue_from *handled do |exception| + ^^^^^^^^^^^ Use `e` instead of `exception`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from *handled do |e| + # do something + end + RUBY + end + end + end + + context 'with the `PreferredName` setup' do + let(:cop_config) do + { + 'PreferredName' => 'exception' + } + end + + it 'does not register an offense when using the preferred name' do + expect_no_offenses(<<~RUBY) + rescue_from MyException do |exception| + # do something + end + RUBY + end + + it 'does not register an offense when using the preferred name with' \ + 'multiple rescued exceptions' do + expect_no_offenses(<<~RUBY) + rescue_from MyException, MyOtherException do |exception| + # do something + end + RUBY + end + + it 'registers an offense when using another name' do + expect_offense(<<~RUBY) + rescue_from MyException do |e| + ^^^ Use `exception` instead of `e`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |exception| + # do something + end + RUBY + end + + it 'registers an offense with underscored prefix variable' do + expect_offense(<<~RUBY) + rescue_from MyException do |_e| + ^^^^ Use `_exception` instead of `_e`. + # do something + end + RUBY + + expect_correction(<<~RUBY) + rescue_from MyException do |_exception| + # do something + end + RUBY + end + end +end