From 75dfabab3df2fbb75fc1f9f25c4a37429e411494 Mon Sep 17 00:00:00 2001 From: Andrew Crump Date: Fri, 27 Sep 2013 17:11:02 +0100 Subject: [PATCH] Parentheses around conditional, refs #91. --- lib/tailor/cli/options.rb | 6 + lib/tailor/configuration/style.rb | 2 + .../rulers/allow_conditional_parentheses.rb | 66 ++++ .../conditional_parentheses_spec.rb | 291 ++++++++++++++++++ spec/support/conditional_parentheses_cases.rb | 60 ++++ spec/unit/tailor/configuration/style_spec.rb | 1 + spec/unit/tailor/configuration_spec.rb | 1 + 7 files changed, 427 insertions(+) create mode 100644 lib/tailor/rulers/allow_conditional_parentheses.rb create mode 100644 spec/functional/conditional_parentheses_spec.rb create mode 100644 spec/support/conditional_parentheses_cases.rb diff --git a/lib/tailor/cli/options.rb b/lib/tailor/cli/options.rb index b53035c..09ee1a3 100644 --- a/lib/tailor/cli/options.rb +++ b/lib/tailor/cli/options.rb @@ -62,6 +62,12 @@ def self.parse!(args) opt.separator "" opt.separator " * Horizontal Spacing:" + + opt.on('--allow-conditional-parentheses BOOL', + 'Check for conditionals wrapped in parentheses? (default: true)') do |c| + options.style[:allow_conditional_parentheses] = c + end + opt.on('--allow-hard-tabs BOOL', 'Check for hard tabs? (default: true)') do |c| options.style[:allow_hard_tabs] = c diff --git a/lib/tailor/configuration/style.rb b/lib/tailor/configuration/style.rb index 85f2618..8130c4f 100644 --- a/lib/tailor/configuration/style.rb +++ b/lib/tailor/configuration/style.rb @@ -26,6 +26,7 @@ def self.define_property(name) end define_property :allow_camel_case_methods + define_property :allow_conditional_parentheses define_property :allow_hard_tabs define_property :allow_screaming_snake_case_classes define_property :allow_trailing_line_spaces @@ -49,6 +50,7 @@ def self.define_property(name) # Sets up default values. def initialize allow_camel_case_methods(false, level: :error) + allow_conditional_parentheses(false, level: :error) allow_hard_tabs(false, level: :error) allow_screaming_snake_case_classes(false, level: :error) allow_trailing_line_spaces(false, level: :error) diff --git a/lib/tailor/rulers/allow_conditional_parentheses.rb b/lib/tailor/rulers/allow_conditional_parentheses.rb new file mode 100644 index 0000000..6d75254 --- /dev/null +++ b/lib/tailor/rulers/allow_conditional_parentheses.rb @@ -0,0 +1,66 @@ +require_relative '../ruler' + +class Tailor + module Rulers + class AllowConditionalParenthesesRuler < Tailor::Ruler + def initialize(style, options) + super(style, options) + add_lexer_observers :nl + end + + def nl_update(current_lexed_line, lineno, column) + measure(current_lexed_line, lineno) + end + + # Checks to see if a conditional is unnecessarily wrapped in parentheses. + # + # @param [Fixnum] line The current lexed line. + # @param [Fixnum] lineno Line the problem was found on. + def measure(line, lineno) + return if @config + return unless line.any? { |t| conditional?(t) } + if tokens_before_lparen?(line) and ! tokens_after_rparen?(line) + column = lparen_column(line) + @problems << Problem.new('conditional_parentheses', lineno, column, + "Parentheses around conditional expression at column #{column}.", + @options[:level]) + end + end + + private + + def conditional?(token) + token[1] == :on_kw and %w{case if unless while}.include?(token[2]) + end + + def lparen?(token) + token[1] == :on_lparen + end + + def lparen_column(tokens) + tokens.find { |t| lparen?(t) }[0][1] + 1 + end + + def tokens_before_lparen?(tokens) + without_spaces( + tokens.select do |t| + true if (conditional?(t))..(lparen?(t)) + end.tap { |t| t.shift; t.pop } + ).empty? + end + + def tokens_after_rparen?(tokens) + without_spaces( + tokens.reverse.tap do |nl| + nl.shift + end.take_while { |t| t[1] != :on_rparen } + ).any? + end + + def without_spaces(tokens) + tokens.reject { |t| t[1] == :on_sp } + end + + end + end +end diff --git a/spec/functional/conditional_parentheses_spec.rb b/spec/functional/conditional_parentheses_spec.rb new file mode 100644 index 0000000..51d2fc6 --- /dev/null +++ b/spec/functional/conditional_parentheses_spec.rb @@ -0,0 +1,291 @@ +require 'spec_helper' +require_relative '../support/conditional_parentheses_cases' +require 'tailor/critic' +require 'tailor/configuration/style' + +describe 'Conditional parentheses' do + + def file_name + self.class.description + end + + def contents + CONDITIONAL_PARENTHESES[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 :no_parentheses do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :with_parentheses do + it 'warns' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 4, + :message=> 'Parentheses around conditional expression at column 4.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :with_parentheses_no_space do + it 'warns' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 3, + :message=> 'Parentheses around conditional expression at column 3.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :method_call do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :indented_method_call do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :method_call_on_parens do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :double_parens do + it 'warns by default' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 4, + :message=> 'Parentheses around conditional expression at column 4.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :unless_no_parentheses do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :unless_with_parentheses do + it 'warns on parentheses' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 8, + :message=> 'Parentheses around conditional expression at column 8.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :case_no_parentheses do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :case_with_parentheses do + it 'warns on parentheses' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 6, + :message=> 'Parentheses around conditional expression at column 6.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :while_no_parentheses do + it 'does not warn' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + + context :while_with_parentheses do + it 'warns on parentheses' do + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to eql [{ + :type => 'conditional_parentheses', + :line => 1, + :column=> 7, + :message=> 'Parentheses around conditional expression at column 7.', + :level=> :error + }] + end + it 'does not warn when parentheses are allowed' do + style.allow_conditional_parentheses true, level: :error + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + it 'does not warn when parentheses are disabled' do + style.allow_conditional_parentheses false, level: :off + critic.check_file(file_name, style.to_hash) + expect(critic.problems[file_name]).to be_empty + end + end + +end diff --git a/spec/support/conditional_parentheses_cases.rb b/spec/support/conditional_parentheses_cases.rb new file mode 100644 index 0000000..bb088de --- /dev/null +++ b/spec/support/conditional_parentheses_cases.rb @@ -0,0 +1,60 @@ +CONDITIONAL_PARENTHESES = {} + +CONDITIONAL_PARENTHESES['no_parentheses'] = + %q{if foo +end} + +CONDITIONAL_PARENTHESES['with_parentheses'] = + %q{if (foo) +end} + +CONDITIONAL_PARENTHESES['with_parentheses_no_space'] = + %q{if(foo) +end} + +CONDITIONAL_PARENTHESES['method_call'] = + %q{if foo(bar) +end} + +CONDITIONAL_PARENTHESES['indented_method_call'] = +%q{foo do + if foo(bar) + end +end} + +CONDITIONAL_PARENTHESES['method_call_on_parens'] = + %q{unless (foo & bar).sort +end +} + +CONDITIONAL_PARENTHESES['double_parens'] = + %q{if ((bar)) +end} + +CONDITIONAL_PARENTHESES['unless_no_parentheses'] = + %q{unless bar +end} + +CONDITIONAL_PARENTHESES['unless_with_parentheses'] = + %q{unless (bar) +end} + +CONDITIONAL_PARENTHESES['case_no_parentheses'] = + %q{case bar +when 1 then 'a' +when 2 then 'b' +end} + +CONDITIONAL_PARENTHESES['case_with_parentheses'] = + %q{case (bar) +when 1 then 'a' +when 2 then 'b' +end} + +CONDITIONAL_PARENTHESES['while_no_parentheses'] = + %q{while bar +end} + +CONDITIONAL_PARENTHESES['while_with_parentheses'] = + %q{while (bar) +end} diff --git a/spec/unit/tailor/configuration/style_spec.rb b/spec/unit/tailor/configuration/style_spec.rb index b852a43..2e6e770 100644 --- a/spec/unit/tailor/configuration/style_spec.rb +++ b/spec/unit/tailor/configuration/style_spec.rb @@ -157,6 +157,7 @@ let(:default_values) do { :allow_camel_case_methods => [false, { :level => :error }], + :allow_conditional_parentheses => [false, { :level => :error }], :allow_hard_tabs => [false, { :level => :error }], :allow_screaming_snake_case_classes => [false, { :level => :error }], :allow_trailing_line_spaces => [false, { :level => :error }], diff --git a/spec/unit/tailor/configuration_spec.rb b/spec/unit/tailor/configuration_spec.rb index 26cb265..1ba0833 100644 --- a/spec/unit/tailor/configuration_spec.rb +++ b/spec/unit/tailor/configuration_spec.rb @@ -40,6 +40,7 @@ file_list: [], style: { allow_camel_case_methods: [false, { level: :error }], + allow_conditional_parentheses: [false, { level: :error }], allow_hard_tabs: [false, { level: :error }], allow_screaming_snake_case_classes: [false, { level: :error }], allow_trailing_line_spaces: [false, { level: :error }],