Merge branch 'unnecessary-string-interpolation' of https://github.com…

…/acrmp/tailor

Relates to gh-149

lib/tailor/cli/options.rb

@@ -81,6 +81,12 @@ def self.parse!(args)
             options.style[:allow_trailing_line_spaces] = c
           end
 
+          opt.on('--allow-unnecessary-interpolation BOOL',
+            'Check for unnecessary interpolation in strings?',
+            '(default: false)') do |c|
+            options.style[:allow_unnecessary_interpolation] = c
+          end
+
           opt.on('--indentation-spaces NUMBER', INTEGER_OR_OFF,
             'Spaces to expect indentation.  (default: 2)') do |c|
             options.style[:indentation_spaces] = c

lib/tailor/configuration/style.rb

@@ -31,6 +31,7 @@ def self.define_property(name)
       define_property :allow_screaming_snake_case_classes
       define_property :allow_trailing_line_spaces
       define_property :allow_invalid_ruby
+      define_property :allow_unnecessary_interpolation
       define_property :indentation_spaces
       define_property :max_code_lines_in_class
       define_property :max_code_lines_in_method
@@ -55,6 +56,7 @@ def initialize
         allow_hard_tabs(false, level: :error)
         allow_screaming_snake_case_classes(false, level: :error)
         allow_trailing_line_spaces(false, level: :error)
+        allow_unnecessary_interpolation(false, level: :warn)
         allow_invalid_ruby(false, level: :warn)
         indentation_spaces(2, level: :error)
         max_code_lines_in_class(300, level: :error)

lib/tailor/rulers/allow_unnecessary_interpolation_ruler.rb

@@ -0,0 +1,87 @@
+require_relative '../ruler'
+
+class Tailor
+  module Rulers
+    class AllowUnnecessaryInterpolationRuler < Tailor::Ruler
+
+      EVENTS = [
+        :on_embexpr_beg,
+        :on_embexpr_end,
+        :on_rbrace,
+        :on_tstring_beg,
+        :on_tstring_content,
+        :on_tstring_end
+      ]
+
+      def initialize(config, options)
+        super(config, options)
+        reset_tokens
+        add_lexer_observers :ignored_nl, :nl
+      end
+
+      def ignored_nl_update(lexed_line, lineno, column)
+        add_string_tokens(lexed_line)
+      end
+
+      def nl_update(lexed_line, lineno, column)
+        add_string_tokens(lexed_line)
+        each_string(@tokens).each do |string|
+          measure(line_number(@tokens.first), string)
+        end
+        reset_tokens
+      end
+
+      # Checks if variables are interpolated unnecessarily
+      #
+      # @param [Array] tokens The filtered tokens
+      def measure(lineno, tokens)
+        return if @config
+        if no_content?(tokens) and one_expression?(tokens)
+          @problems << Problem.new('unnecessary_string_interpolation', lineno,
+            column(tokens.first), 'Variable interpolated unnecessarily',
+            @options[:level])
+        end
+      end
+
+      private
+
+      def add_string_tokens(lexed_line)
+        @tokens += string_tokens(lexed_line)
+      end
+
+      def column(token)
+        token.first.last + 1
+      end
+
+      def each_string(tokens)
+        tokens.select do |t|
+          true if (t[1] == :on_tstring_beg)..(t[1] == :on_tstring_end)
+        end.slice_before { |t| t[1] == :on_tstring_beg }
+      end
+
+      def line_number(token)
+        token.first.first
+      end
+
+      def no_content?(tokens)
+        ! tokens.map { |t| t[1] }.include?(:on_tstring_content)
+      end
+
+      def one_expression?(tokens)
+        tokens.select { |t| t[1] == :on_embexpr_beg }.size == 1 and
+          tokens.select do |t|
+            t[1] == :on_embexpr_end or t[1] == :on_rbrace
+          end.any?
+      end
+
+      def reset_tokens
+        @tokens = []
+      end
+
+      def string_tokens(lexed_line)
+        lexed_line.select { |t| EVENTS.include?(t[1]) }
+      end
+
+    end
+  end
+end

spec/functional/horizontal_spacing/braces_spec.rb

@@ -224,15 +224,16 @@
 
     context "no space before consecutive rbraces" do
       let(:file_name) { 'no_space_before_consecutive_rbraces' }
-      specify { critic.problems[file_name].size.should be 2 }
-      specify { critic.problems[file_name].first[:type].should == "spaces_before_rbrace" }
-      specify { critic.problems[file_name].first[:line].should be 1 }
-      specify { critic.problems[file_name].first[:column].should be 72 }
-      specify { critic.problems[file_name].first[:level].should be :error }
-      specify { critic.problems[file_name].last[:type].should == "spaces_before_rbrace" }
-      specify { critic.problems[file_name].last[:line].should be 1 }
-      specify { critic.problems[file_name].last[:column].should be 73 }
-      specify { critic.problems[file_name].last[:level].should be :error }
+      let(:problems) { critic.problems[file_name].select { |p| p[:type] == 'spaces_before_rbrace' } }
+      specify { problems.size.should be 2 }
+      specify { problems.first[:type].should == "spaces_before_rbrace" }
+      specify { problems.first[:line].should be 1 }
+      specify { problems.first[:column].should be 72 }
+      specify { problems.first[:level].should be :error }
+      specify { problems.last[:type].should == "spaces_before_rbrace" }
+      specify { problems.last[:line].should be 1 }
+      specify { problems.last[:column].should be 73 }
+      specify { problems.last[:level].should be :error }
     end
   end
 end

spec/functional/string_interpolation_spec.rb

@@ -0,0 +1,117 @@
+require 'spec_helper'
+require_relative '../support/string_interpolation_cases'
+require 'tailor/critic'
+require 'tailor/configuration/style'
+
+describe 'String interpolation cases' do
+
+  def file_name
+    self.class.description
+  end
+
+  def contents
+    INTERPOLATION[file_name] || begin
+      raise "Example not found: #{file_name}"
+    end
+  end
+
+  before do
+    Tailor::Logger.stub(:log)
+    FakeFS.activate!
+    FileUtils.touch file_name
+    File.open(file_name, 'w') { |f| f.write contents }
+  end
+
+  let(:critic) { Tailor::Critic.new }
+
+  let(:style) do
+    style = Tailor::Configuration::Style.new
+    style.trailing_newlines 0, level: :off
+    style.allow_invalid_ruby true, level: :off
+    style
+  end
+
+  context :one_variable_interpolated_only do
+    it 'warns when interpolation is used unnecessarily' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to eql [{
+        :type => 'unnecessary_string_interpolation',
+        :line => 1,
+        :column=> 6,
+        :message=> 'Variable interpolated unnecessarily',
+        :level=> :warn
+      }]
+    end
+  end
+
+  context :mixed_content_and_expression do
+    it 'does not warn' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to be_empty
+    end
+  end
+
+  context :no_string do
+    it 'does not warn' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to be_empty
+    end
+  end
+
+  context :two_variables do
+    it 'does not warn' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to be_empty
+    end
+  end
+
+  context :two_strings_with_unnecessary_interpolation do
+    it 'warns against both strings' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to eql [
+        {
+          :type => 'unnecessary_string_interpolation',
+          :line => 1,
+          :column=> 6,
+          :message=> 'Variable interpolated unnecessarily',
+          :level=> :warn
+        },
+        {
+          :type => 'unnecessary_string_interpolation',
+          :line => 1,
+          :column=> 17,
+          :message=> 'Variable interpolated unnecessarily',
+          :level=> :warn
+        }
+      ]
+    end
+  end
+
+  context :multiline_string_with_unnecessary_interpolation do
+    it 'warns against the first line' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to eql [{
+        :type => 'unnecessary_string_interpolation',
+        :line => 1,
+        :column=> 6,
+        :message=> 'Variable interpolated unnecessarily',
+        :level=> :warn
+      }]
+    end
+  end
+
+  context :multiline_word_list do
+    it 'does not warn' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to be_empty
+    end
+  end
+
+  context :nested_interpolation do
+    it 'does not warn' do
+      critic.check_file(file_name, style.to_hash)
+      expect(critic.problems[file_name]).to be_empty
+    end
+  end
+
+end

spec/support/good_indentation_cases.rb

@@ -143,7 +143,7 @@ class MyError < RuntimeError; end
 rescue SocketError, ArgumentError, SystemCallError,
   Net::SCP::Exception, Timeout::Error => ex
   @logger.error "Failed to copy the file \#{source} to \#{dest} due to " +
-    "\#{ex.message}"
+    ex.message
 end}
 
 INDENT_OK['rescue_ending_with_comma_trailing_comment'] =
@@ -154,7 +154,7 @@ class MyError < RuntimeError; end
 rescue SocketError, ArgumentError, SystemCallError,     # comment
   Net::SCP::Exception, Timeout::Error => ex
   @logger.error "Failed to copy the file \#{source} to \#{dest} due to " +
-    "\#{ex.message}"
+    ex.message
 end}
 
 INDENT_OK['def_rescue'] =
@@ -738,7 +738,7 @@ def foo
 
 INDENT_OK['combo3'] =
   %Q{def report_turducken(results, performance_results)
-  stuffing[:log_files] = { "\#{File.basename @logger.log_file_location}" =>
+  stuffing[:log_files] = { "\#{File.basename @logger.log_file_location}/path" =>
     File.read(@logger.log_file_location).gsub(/(?<f><)(?<q>\\/)?(?<w>\\w)/,
       '\\k<f>!\\k<q>\\k<w>') }.merge remote_logs
 

spec/support/horizontal_spacing_cases.rb

@@ -87,7 +87,7 @@
   %Q{[1].map { |n| { :first => "\#{n}-\#{{}}" } }}
 
 H_SPACING_OK['string_interp_with_colonop'] =
-  %Q{"\#{::Rails.root}"}
+  %Q{"\#{::Rails.root + 'file'}"}
 
 
 

spec/support/string_interpolation_cases.rb

@@ -0,0 +1,45 @@
+INTERPOLATION = {}
+
+INTERPOLATION['one_variable_interpolated_only'] =
+  %q{puts "#{bing}"
+}
+
+INTERPOLATION['mixed_content_and_expression'] =
+  %q{puts "hello: #{bing}"
+}
+
+INTERPOLATION['no_string'] =
+  %q{puts bing
+}
+
+INTERPOLATION['two_variables'] =
+  %q{puts "#{bing}#{bar}"
+}
+
+INTERPOLATION['two_strings_with_unnecessary_interpolation'] =
+  %q{puts "#{foo}" + "#{bar}"
+}
+
+INTERPOLATION['multiline_string_with_unnecessary_interpolation'] =
+  %q{puts "#{foo +
+bar -
+baz}"
+}
+
+INTERPOLATION['multiline_word_list'] =
+  %q{%w{
+  foo
+  bar
+  baz
+}}
+
+INTERPOLATION['nested_interpolation'] =
+  %q[def friendly_time(time)
+  if hours < 24
+    "#{(hours > 0) ? "#{hours} hour" : '' }#{(hours > 1) ? 's' : ''}" +
+      " #{(mins > 0) ? "#{mins} minute" : '' }#{(mins > 1) ? 's' : ''}" +
+      " #{seconds} second#{(seconds > 1) ? "s" : ''} ago"
+  else
+    time.to_s
+  end
+end]

spec/unit/tailor/configuration/style_spec.rb

@@ -161,6 +161,7 @@
         :allow_hard_tabs => [false, { :level => :error }],
         :allow_screaming_snake_case_classes => [false, { :level => :error }],
         :allow_trailing_line_spaces => [false, { :level => :error }],
+        :allow_unnecessary_interpolation => [false, { :level => :warn }],
         :allow_invalid_ruby => [false, { :level => :warn }],
         :indentation_spaces => [2, { :level => :error }],
         :max_code_lines_in_class => [300, { :level => :error }],

spec/unit/tailor/configuration_spec.rb

@@ -44,6 +44,7 @@
             allow_hard_tabs: [false, { level: :error }],
             allow_screaming_snake_case_classes: [false, { level: :error }],
             allow_trailing_line_spaces: [false, { level: :error }],
+            allow_unnecessary_interpolation: [false, { level: :warn }],
             allow_invalid_ruby: [false, { level: :warn }],
             indentation_spaces: [2, { level: :error }],
             max_code_lines_in_class: [300, { level: :error }],