diff --git a/CHANGELOG.md b/CHANGELOG.md index 772bdc0217..41e7deaf57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug fixes * [#180](https://github.com/rubocop-hq/rubocop-rails/issues/180): Fix a false positive for `HttpPositionalArguments` when using `get` method with `:to` option. ([@koic][]) +* [#193](https://github.com/rubocop-hq/rubocop-rails/issues/193): Make `Rails/EnvironmentComparison` aware of `Rails.env` is used in RHS or when `!=` is used for comparison. ([@koic][]) ## 2.4.2 (2020-01-26) diff --git a/lib/rubocop/cop/rails/environment_comparison.rb b/lib/rubocop/cop/rails/environment_comparison.rb index 4f29a7a7a0..154b3df91e 100644 --- a/lib/rubocop/cop/rails/environment_comparison.rb +++ b/lib/rubocop/cop/rails/environment_comparison.rb @@ -16,52 +16,98 @@ module Rails # # good # Rails.env.production? class EnvironmentComparison < Cop - MSG = "Favor `Rails.env.%s?` over `Rails.env == '%s'`." + MSG = 'Favor `%sRails.env.%s?` over `%s`.' SYM_MSG = 'Do not compare `Rails.env` with a symbol, it will always ' \ 'evaluate to `false`.' - def_node_matcher :environment_str_comparison?, <<~PATTERN + def_node_matcher :comparing_str_env_with_rails_env_on_lhs?, <<~PATTERN (send (send (const {nil? cbase} :Rails) :env) - :== + {:== :!=} $str ) PATTERN - def_node_matcher :environment_sym_comparison?, <<~PATTERN + def_node_matcher :comparing_str_env_with_rails_env_on_rhs?, <<~PATTERN + (send + $str + {:== :!=} + (send (const {nil? cbase} :Rails) :env) + ) + PATTERN + + def_node_matcher :comparing_sym_env_with_rails_env_on_lhs?, <<~PATTERN (send (send (const {nil? cbase} :Rails) :env) - :== + {:== :!=} $sym ) PATTERN + def_node_matcher :comparing_sym_env_with_rails_env_on_rhs?, <<~PATTERN + (send + $sym + {:== :!=} + (send (const {nil? cbase} :Rails) :env) + ) + PATTERN + + def_node_matcher :content, <<~PATTERN + ({str sym} $_) + PATTERN + def on_send(node) - environment_str_comparison?(node) do |env_node| + if (env_node = comparing_str_env_with_rails_env_on_lhs?(node) || + comparing_str_env_with_rails_env_on_rhs?(node)) env, = *env_node - add_offense(node, message: format(MSG, env: env)) + bang = node.method?(:!=) ? '!' : '' + + add_offense(node, message: format( + MSG, bang: bang, env: env, source: node.source + )) end - environment_sym_comparison?(node) do |_| + + if comparing_sym_env_with_rails_env_on_lhs?(node) || + comparing_sym_env_with_rails_env_on_rhs?(node) add_offense(node, message: SYM_MSG) end end def autocorrect(node) lambda do |corrector| - corrector.replace(node.source_range, replacement(node)) + replacement = build_predicate_method(node) + + corrector.replace(node.source_range, replacement) end end private - def replacement(node) - "#{node.receiver.source}.#{content(node.first_argument)}?" + def build_predicate_method(node) + if rails_env_on_lhs?(node) + build_predicate_method_for_rails_env_on_lhs(node) + else + build_predicate_method_for_rails_env_on_rhs(node) + end end - def_node_matcher :content, <<~PATTERN - ({str sym} $_) - PATTERN + def rails_env_on_lhs?(node) + comparing_str_env_with_rails_env_on_lhs?(node) || + comparing_sym_env_with_rails_env_on_lhs?(node) + end + + def build_predicate_method_for_rails_env_on_lhs(node) + bang = node.method?(:!=) ? '!' : '' + + "#{bang}#{node.receiver.source}.#{content(node.first_argument)}?" + end + + def build_predicate_method_for_rails_env_on_rhs(node) + bang = node.method?(:!=) ? '!' : '' + + "#{bang}#{node.first_argument.source}.#{content(node.receiver)}?" + end end end end diff --git a/spec/rubocop/cop/rails/environment_comparison_spec.rb b/spec/rubocop/cop/rails/environment_comparison_spec.rb index f628391cbf..0a589dfa60 100644 --- a/spec/rubocop/cop/rails/environment_comparison_spec.rb +++ b/spec/rubocop/cop/rails/environment_comparison_spec.rb @@ -5,26 +5,104 @@ let(:config) { RuboCop::Config.new } - it 'registers an offense and corrects comparing Rails.env to a string' do - expect_offense(<<~RUBY) - Rails.env == 'production' - ^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `Rails.env.production?` over `Rails.env == 'production'`. - RUBY + context 'when comparing `Rails.env` to a string' do + context 'when using equals' do + it 'registers an offense and corrects when `Rails.env` is used on LHS' do + expect_offense(<<~RUBY) + Rails.env == 'production' + ^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `Rails.env.production?` over `Rails.env == 'production'`. + RUBY - expect_correction(<<~RUBY) - Rails.env.production? - RUBY + expect_correction(<<~RUBY) + Rails.env.production? + RUBY + end + + it 'registers an offense and corrects when `Rails.env` is used on RHS' do + expect_offense(<<~RUBY) + 'production' == Rails.env + ^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `Rails.env.production?` over `'production' == Rails.env`. + RUBY + + expect_correction(<<~RUBY) + Rails.env.production? + RUBY + end + end + + context 'when using not equals' do + it 'registers an offense and corrects when `Rails.env` is used on LHS' do + expect_offense(<<~RUBY) + Rails.env != 'production' + ^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `!Rails.env.production?` over `Rails.env != 'production'`. + RUBY + + expect_correction(<<~RUBY) + !Rails.env.production? + RUBY + end + + it 'registers an offense and corrects when `Rails.env` is used on RHS' do + expect_offense(<<~RUBY) + 'production' != Rails.env + ^^^^^^^^^^^^^^^^^^^^^^^^^ Favor `!Rails.env.production?` over `'production' != Rails.env`. + RUBY + + expect_correction(<<~RUBY) + !Rails.env.production? + RUBY + end + end end - it 'registers an offense and corrects comparing Rails.env to a symbol' do - expect_offense(<<~RUBY) - Rails.env == :production - ^^^^^^^^^^^^^^^^^^^^^^^^ Do not compare `Rails.env` with a symbol, it will always evaluate to `false`. - RUBY + context 'when comparing `Rails.env` to a symbol' do + context 'when using equals' do + it 'registers an offense and corrects when `Rails.env` is used on LHS' do + expect_offense(<<~RUBY) + Rails.env == :production + ^^^^^^^^^^^^^^^^^^^^^^^^ Do not compare `Rails.env` with a symbol, it will always evaluate to `false`. + RUBY - expect_correction(<<~RUBY) - Rails.env.production? - RUBY + expect_correction(<<~RUBY) + Rails.env.production? + RUBY + end + + it 'registers an offense and corrects when `Rails.env` is used on RHS' do + expect_offense(<<~RUBY) + :production == Rails.env + ^^^^^^^^^^^^^^^^^^^^^^^^ Do not compare `Rails.env` with a symbol, it will always evaluate to `false`. + RUBY + + expect_correction(<<~RUBY) + Rails.env.production? + RUBY + end + end + + context 'when using not equals' do + it 'registers an offense and corrects when `Rails.env` is used on LHS' do + expect_offense(<<~RUBY) + Rails.env != :production + ^^^^^^^^^^^^^^^^^^^^^^^^ Do not compare `Rails.env` with a symbol, it will always evaluate to `false`. + RUBY + + expect_correction(<<~RUBY) + !Rails.env.production? + RUBY + end + + it 'registers an offense and corrects when `Rails.env` is used on RHS' do + expect_offense(<<~RUBY) + :production != Rails.env + ^^^^^^^^^^^^^^^^^^^^^^^^ Do not compare `Rails.env` with a symbol, it will always evaluate to `false`. + RUBY + + expect_correction(<<~RUBY) + !Rails.env.production? + RUBY + end + end end it 'does not register an offense when using `#good_method`' do