[Fix rubocop#193] Make Rails/EnvironmentComparison aware of several…

… comparisons Fixes rubocop#193. This PR makes `Rails/EnvironmentComparison` aware of the following comparisons: - When `Rails.env` is used on RHS - When `!=` is used to compare with `Rails.env`
koic · Feb 4, 2020 · 87fdbe2 · 87fdbe2
1 parent da64cc2
commit 87fdbe2
Show file tree

Hide file tree

Showing 3 changed files with 155 additions and 30 deletions.
diff --git 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
@@ -16,52 +16,98 @@ module Rails
       #   # good
       #   Rails.env.production?
       class EnvironmentComparison < Cop
-        MSG = "Favor `Rails.env.%<env>s?` over `Rails.env == '%<env>s'`."
+        MSG = 'Favor `%<bang>sRails.env.%<env>s?` over `%<source>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
@@ -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