diff --git a/lib/tailor/cli/options.rb b/lib/tailor/cli/options.rb index 9edd916..1dce086 100644 --- a/lib/tailor/cli/options.rb +++ b/lib/tailor/cli/options.rb @@ -75,6 +75,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 diff --git a/lib/tailor/configuration/style.rb b/lib/tailor/configuration/style.rb index 873fcbb..28cacdb 100644 --- a/lib/tailor/configuration/style.rb +++ b/lib/tailor/configuration/style.rb @@ -30,6 +30,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 @@ -53,6 +54,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) diff --git a/lib/tailor/rulers/allow_unnecessary_interpolation_ruler.rb b/lib/tailor/rulers/allow_unnecessary_interpolation_ruler.rb new file mode 100644 index 0000000..bf1a548 --- /dev/null +++ b/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 diff --git a/spec/functional/horizontal_spacing/braces_spec.rb b/spec/functional/horizontal_spacing/braces_spec.rb index 7cbfa3b..cfcbb12 100644 --- a/spec/functional/horizontal_spacing/braces_spec.rb +++ b/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 diff --git a/spec/functional/string_interpolation_spec.rb b/spec/functional/string_interpolation_spec.rb new file mode 100644 index 0000000..214953f --- /dev/null +++ b/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 diff --git a/spec/support/good_indentation_cases.rb b/spec/support/good_indentation_cases.rb index e973b86..fc6715c 100644 --- a/spec/support/good_indentation_cases.rb +++ b/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(/(?<)(?\\/)?(?\\w)/, '\\k!\\k\\k') }.merge remote_logs diff --git a/spec/support/horizontal_spacing_cases.rb b/spec/support/horizontal_spacing_cases.rb index b6ec05b..93e9504 100644 --- a/spec/support/horizontal_spacing_cases.rb +++ b/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'}"} diff --git a/spec/support/string_interpolation_cases.rb b/spec/support/string_interpolation_cases.rb new file mode 100644 index 0000000..a01742f --- /dev/null +++ b/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] diff --git a/spec/unit/tailor/configuration/style_spec.rb b/spec/unit/tailor/configuration/style_spec.rb index 1f49c24..21e98bc 100644 --- a/spec/unit/tailor/configuration/style_spec.rb +++ b/spec/unit/tailor/configuration/style_spec.rb @@ -160,6 +160,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 }], diff --git a/spec/unit/tailor/configuration_spec.rb b/spec/unit/tailor/configuration_spec.rb index 3cffc9e..82f8beb 100644 --- a/spec/unit/tailor/configuration_spec.rb +++ b/spec/unit/tailor/configuration_spec.rb @@ -43,6 +43,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 }],