From 4f9c16b38b08ad4aad65203c90ac6fecd7200821 Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Sat, 29 Dec 2012 21:22:34 +0100 Subject: [PATCH 1/8] Added DefParentheses cop. --- lib/rubocop.rb | 1 + lib/rubocop/cop/def_parentheses.rb | 35 +++++++++++++++++++ spec/rubocop/cops/def_parentheses_spec.rb | 42 +++++++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 lib/rubocop/cop/def_parentheses.rb create mode 100644 spec/rubocop/cops/def_parentheses_spec.rb diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 760786bd7b4..0b85b888dab 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -16,6 +16,7 @@ require 'rubocop/cop/end_of_line' require 'rubocop/cop/numeric_literals' require 'rubocop/cop/align_parameters' +require 'rubocop/cop/def_parentheses' require 'rubocop/report/report' require 'rubocop/report/plain_text' diff --git a/lib/rubocop/cop/def_parentheses.rb b/lib/rubocop/cop/def_parentheses.rb new file mode 100644 index 00000000000..4a0aa05556c --- /dev/null +++ b/lib/rubocop/cop/def_parentheses.rb @@ -0,0 +1,35 @@ +# encoding: utf-8 + +module Rubocop + module Cop + class DefParentheses < Cop + ERROR_MESSAGE = ['Use def with parentheses when there are arguments.', + "Omit the parentheses in defs when the method " + + "doesn't accept any arguments."] + EMPTY_PARAMS = [:params, nil, nil, nil, nil, nil] + + def inspect(file, source, tokens, sexp) + each(:def, sexp) do |def_sexp| + case def_sexp[2][0] + when :params + if def_sexp[2] != EMPTY_PARAMS + add(def_sexp, source, ERROR_MESSAGE[0]) + end + when :paren + if def_sexp[2][1] == EMPTY_PARAMS + add(def_sexp, source, ERROR_MESSAGE[1]) + end + end + end + end + + private + + def add(def_sexp, source, message) + pos = def_sexp[1][-1] + index = pos[0] - 1 + add_offence(:convention, index, source[index], message) + end + end + end +end diff --git a/spec/rubocop/cops/def_parentheses_spec.rb b/spec/rubocop/cops/def_parentheses_spec.rb new file mode 100644 index 00000000000..6f48c639f55 --- /dev/null +++ b/spec/rubocop/cops/def_parentheses_spec.rb @@ -0,0 +1,42 @@ +# encoding: utf-8 + +require 'spec_helper' + +module Rubocop + module Cop + describe DefParentheses do + let (:def_par) { DefParentheses.new } + + it 'reports an offence for def with parameters but no parens' do + src = ['def func a, b', + 'end'] + def_par.inspect_source('', src) + def_par.offences.map(&:message).should == + ['Use def with parentheses when there are arguments.'] + end + + it 'reports an offence for def with empty parens' do + src = ['def func()', + 'end'] + def_par.inspect_source('', src) + def_par.offences.map(&:message).should == + ["Omit the parentheses in defs when the method doesn't accept any " + + "arguments."] + end + + it 'accepts def with arg and parens' do + src = ['def func(a)', + 'end'] + def_par.inspect_source('', src) + def_par.offences.map(&:message).should == [] + end + + it 'accepts def with no args and no parens' do + src = ['def func', + 'end'] + def_par.inspect_source('', src) + def_par.offences.map(&:message).should == [] + end + end + end +end From c973550a9a9b90ca2347344e3c0b58d1e36e230a Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Sun, 30 Dec 2012 07:04:27 +0100 Subject: [PATCH 2/8] Accepting empty parens in one line defs. --- lib/rubocop/cop/def_parentheses.rb | 20 ++++++++++++++------ spec/rubocop/cops/def_parentheses_spec.rb | 6 ++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cop/def_parentheses.rb b/lib/rubocop/cop/def_parentheses.rb index 4a0aa05556c..a12011c7ad5 100644 --- a/lib/rubocop/cop/def_parentheses.rb +++ b/lib/rubocop/cop/def_parentheses.rb @@ -10,14 +10,23 @@ class DefParentheses < Cop def inspect(file, source, tokens, sexp) each(:def, sexp) do |def_sexp| + pos = def_sexp[1][-1] case def_sexp[2][0] when :params - if def_sexp[2] != EMPTY_PARAMS - add(def_sexp, source, ERROR_MESSAGE[0]) - end + add(pos, source, ERROR_MESSAGE[0]) if def_sexp[2] != EMPTY_PARAMS when :paren if def_sexp[2][1] == EMPTY_PARAMS - add(def_sexp, source, ERROR_MESSAGE[1]) + method_name_ix = tokens.index { |t| t[0] == pos } + start = method_name_ix + 1 + rparen_ix = start + tokens[start..-1].index { |t| t[2] == ')' } + first_body_token = tokens[(rparen_ix + 1)..-1].find do |t| + not whitespace?(t) + end + if first_body_token[0][0] > pos[0] + # Only report offence if there's a line break after + # the empty parens. + add(pos, source, ERROR_MESSAGE[1]) + end end end end @@ -25,8 +34,7 @@ def inspect(file, source, tokens, sexp) private - def add(def_sexp, source, message) - pos = def_sexp[1][-1] + def add(pos, source, message) index = pos[0] - 1 add_offence(:convention, index, source[index], message) end diff --git a/spec/rubocop/cops/def_parentheses_spec.rb b/spec/rubocop/cops/def_parentheses_spec.rb index 6f48c639f55..354c6412a00 100644 --- a/spec/rubocop/cops/def_parentheses_spec.rb +++ b/spec/rubocop/cops/def_parentheses_spec.rb @@ -37,6 +37,12 @@ module Cop def_par.inspect_source('', src) def_par.offences.map(&:message).should == [] end + + it 'accepts empty parentheses in one liners' do + src = ["def to_s() join '/' end"] + def_par.inspect_source('', src) + def_par.offences.map(&:message).should == [] + end end end end From b1b439b5c36692261f638502e892fd9f180a9b2e Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Tue, 1 Jan 2013 11:53:42 +0100 Subject: [PATCH 3/8] Added UnnecessaryThen cop. --- lib/rubocop.rb | 1 + lib/rubocop/cop/unnecessary_then.rb | 39 +++++++++++++++++ spec/rubocop/cops/unnecessary_then_spec.rb | 51 ++++++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 lib/rubocop/cop/unnecessary_then.rb create mode 100644 spec/rubocop/cops/unnecessary_then_spec.rb diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 0b85b888dab..17cf803120f 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -17,6 +17,7 @@ require 'rubocop/cop/numeric_literals' require 'rubocop/cop/align_parameters' require 'rubocop/cop/def_parentheses' +require 'rubocop/cop/unnecessary_then' require 'rubocop/report/report' require 'rubocop/report/plain_text' diff --git a/lib/rubocop/cop/unnecessary_then.rb b/lib/rubocop/cop/unnecessary_then.rb new file mode 100644 index 00000000000..20d08db7d0e --- /dev/null +++ b/lib/rubocop/cop/unnecessary_then.rb @@ -0,0 +1,39 @@ +# encoding: utf-8 + +module Rubocop + module Cop + class UnnecessaryThen < Cop + ERROR_MESSAGE = 'Never use then for multi-line if/unless.' + + def inspect(file, source, tokens, sexp) + tokens.each_with_index do |t, ix| + if t[1] == :on_kw && ['if', 'unless'].include?(t[2]) + if multiline_if_then?(tokens, ix + 1) + index = t[0][0] - 1 + add_offence(:convention, index, source[index], ERROR_MESSAGE) + end + end + end + end + + def multiline_if_then?(tokens, ix) + end_found = then_found = false + tokens[ix..-1].each do |t| + case t[1] + when :on_kw + case t[2] + when 'then' + then_found = true + when 'end' + end_found = true + break + end + when :on_ignored_nl, :on_nl + break + end + end + then_found && !end_found + end + end + end +end diff --git a/spec/rubocop/cops/unnecessary_then_spec.rb b/spec/rubocop/cops/unnecessary_then_spec.rb new file mode 100644 index 00000000000..47ee177eb35 --- /dev/null +++ b/spec/rubocop/cops/unnecessary_then_spec.rb @@ -0,0 +1,51 @@ +# encoding: utf-8 + +require 'spec_helper' + +module Rubocop + module Cop + describe UnnecessaryThen do + let (:un_then) { UnnecessaryThen.new } + + # if + + it 'registers an offence for then in multiline if' do + un_then.inspect_source('', ['if cond then', + 'end']) + un_then.offences.map(&:message).sort.should == + ['Never use then for multi-line if/unless.'] + end + + it 'accepts multiline if without then' do + un_then.inspect_source('', ['if cond', + 'end']) + un_then.offences.map(&:message).sort.should == [] + end + + it 'accepts one line if/then/ends' do + un_then.inspect_source('', ['if cond then run end']) + un_then.offences.map(&:message).sort.should == [] + end + + # unless + + it 'registers an offence for then in multiline unless' do + un_then.inspect_source('', ['unless cond then', + 'end']) + un_then.offences.map(&:message).sort.should == + ['Never use then for multi-line if/unless.'] + end + + it 'accepts multiline unless without then' do + un_then.inspect_source('', ['unless cond', + 'end']) + un_then.offences.map(&:message).sort.should == [] + end + + it 'accepts one line unless/then/ends' do + un_then.inspect_source('', ['unless cond then run end']) + un_then.offences.map(&:message).sort.should == [] + end + end + end +end From fc8d633a247ead687a7e5942e89def06fea3533d Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Tue, 1 Jan 2013 13:33:37 +0100 Subject: [PATCH 4/8] Necessary updates after rebase from master. --- lib/rubocop/cop/def_parentheses.rb | 8 ++++---- lib/rubocop/cop/unnecessary_then.rb | 19 ++++++++----------- spec/rubocop/cops/def_parentheses_spec.rb | 10 +++++----- spec/rubocop/cops/unnecessary_then_spec.rb | 12 ++++++------ 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/lib/rubocop/cop/def_parentheses.rb b/lib/rubocop/cop/def_parentheses.rb index a12011c7ad5..a3c70b63d8a 100644 --- a/lib/rubocop/cop/def_parentheses.rb +++ b/lib/rubocop/cop/def_parentheses.rb @@ -16,13 +16,13 @@ def inspect(file, source, tokens, sexp) add(pos, source, ERROR_MESSAGE[0]) if def_sexp[2] != EMPTY_PARAMS when :paren if def_sexp[2][1] == EMPTY_PARAMS - method_name_ix = tokens.index { |t| t[0] == pos } + method_name_ix = tokens.index { |t| t.pos == pos } start = method_name_ix + 1 - rparen_ix = start + tokens[start..-1].index { |t| t[2] == ')' } + rparen_ix = start + tokens[start..-1].index { |t| t.text == ')' } first_body_token = tokens[(rparen_ix + 1)..-1].find do |t| not whitespace?(t) end - if first_body_token[0][0] > pos[0] + if first_body_token.pos.lineno > pos.lineno # Only report offence if there's a line break after # the empty parens. add(pos, source, ERROR_MESSAGE[1]) @@ -35,7 +35,7 @@ def inspect(file, source, tokens, sexp) private def add(pos, source, message) - index = pos[0] - 1 + index = pos.lineno - 1 add_offence(:convention, index, source[index], message) end end diff --git a/lib/rubocop/cop/unnecessary_then.rb b/lib/rubocop/cop/unnecessary_then.rb index 20d08db7d0e..32e8b67fe62 100644 --- a/lib/rubocop/cop/unnecessary_then.rb +++ b/lib/rubocop/cop/unnecessary_then.rb @@ -7,9 +7,9 @@ class UnnecessaryThen < Cop def inspect(file, source, tokens, sexp) tokens.each_with_index do |t, ix| - if t[1] == :on_kw && ['if', 'unless'].include?(t[2]) + if t.type == :on_kw && ['if', 'unless'].include?(t.text) if multiline_if_then?(tokens, ix + 1) - index = t[0][0] - 1 + index = t.pos.lineno - 1 add_offence(:convention, index, source[index], ERROR_MESSAGE) end end @@ -17,22 +17,19 @@ def inspect(file, source, tokens, sexp) end def multiline_if_then?(tokens, ix) - end_found = then_found = false + then_found = false tokens[ix..-1].each do |t| - case t[1] + case t.type when :on_kw - case t[2] - when 'then' - then_found = true - when 'end' - end_found = true - break + case t.text + when 'then' then then_found = true + when 'end' then return false end when :on_ignored_nl, :on_nl break end end - then_found && !end_found + then_found end end end diff --git a/spec/rubocop/cops/def_parentheses_spec.rb b/spec/rubocop/cops/def_parentheses_spec.rb index 354c6412a00..eb174d6300d 100644 --- a/spec/rubocop/cops/def_parentheses_spec.rb +++ b/spec/rubocop/cops/def_parentheses_spec.rb @@ -10,7 +10,7 @@ module Cop it 'reports an offence for def with parameters but no parens' do src = ['def func a, b', 'end'] - def_par.inspect_source('', src) + inspect_source(def_par, '', src) def_par.offences.map(&:message).should == ['Use def with parentheses when there are arguments.'] end @@ -18,7 +18,7 @@ module Cop it 'reports an offence for def with empty parens' do src = ['def func()', 'end'] - def_par.inspect_source('', src) + inspect_source(def_par, '', src) def_par.offences.map(&:message).should == ["Omit the parentheses in defs when the method doesn't accept any " + "arguments."] @@ -27,20 +27,20 @@ module Cop it 'accepts def with arg and parens' do src = ['def func(a)', 'end'] - def_par.inspect_source('', src) + inspect_source(def_par, '', src) def_par.offences.map(&:message).should == [] end it 'accepts def with no args and no parens' do src = ['def func', 'end'] - def_par.inspect_source('', src) + inspect_source(def_par, '', src) def_par.offences.map(&:message).should == [] end it 'accepts empty parentheses in one liners' do src = ["def to_s() join '/' end"] - def_par.inspect_source('', src) + inspect_source(def_par, '', src) def_par.offences.map(&:message).should == [] end end diff --git a/spec/rubocop/cops/unnecessary_then_spec.rb b/spec/rubocop/cops/unnecessary_then_spec.rb index 47ee177eb35..da1dfa746fc 100644 --- a/spec/rubocop/cops/unnecessary_then_spec.rb +++ b/spec/rubocop/cops/unnecessary_then_spec.rb @@ -10,40 +10,40 @@ module Cop # if it 'registers an offence for then in multiline if' do - un_then.inspect_source('', ['if cond then', + inspect_source(un_then, '', ['if cond then', 'end']) un_then.offences.map(&:message).sort.should == ['Never use then for multi-line if/unless.'] end it 'accepts multiline if without then' do - un_then.inspect_source('', ['if cond', + inspect_source(un_then, '', ['if cond', 'end']) un_then.offences.map(&:message).sort.should == [] end it 'accepts one line if/then/ends' do - un_then.inspect_source('', ['if cond then run end']) + inspect_source(un_then, '', ['if cond then run end']) un_then.offences.map(&:message).sort.should == [] end # unless it 'registers an offence for then in multiline unless' do - un_then.inspect_source('', ['unless cond then', + inspect_source(un_then, '', ['unless cond then', 'end']) un_then.offences.map(&:message).sort.should == ['Never use then for multi-line if/unless.'] end it 'accepts multiline unless without then' do - un_then.inspect_source('', ['unless cond', + inspect_source(un_then, '', ['unless cond', 'end']) un_then.offences.map(&:message).sort.should == [] end it 'accepts one line unless/then/ends' do - un_then.inspect_source('', ['unless cond then run end']) + inspect_source(un_then, '', ['unless cond then run end']) un_then.offences.map(&:message).sort.should == [] end end From 55e093d1a5566d1cbc8f196439043e9a229c1783 Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Tue, 1 Jan 2013 16:07:40 +0100 Subject: [PATCH 5/8] Accepting table style if/then/elsif/ends. --- lib/rubocop/cop/cop.rb | 4 ++++ lib/rubocop/cop/unnecessary_then.rb | 6 +++++ spec/rubocop/cops/unnecessary_then_spec.rb | 27 +++++++++++++++++----- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cop/cop.rb b/lib/rubocop/cop/cop.rb index f4f24d29c1f..bfb671a220e 100644 --- a/lib/rubocop/cop/cop.rb +++ b/lib/rubocop/cop/cop.rb @@ -24,6 +24,10 @@ class Token def initialize(pos, type, text) @pos, @type, @text = Position.new(*pos), type, text end + + def to_s + "[[#{@pos.lineno}, #{@pos.column}], #@type, #{@text.inspect}]" + end end class Cop diff --git a/lib/rubocop/cop/unnecessary_then.rb b/lib/rubocop/cop/unnecessary_then.rb index 32e8b67fe62..baf57754509 100644 --- a/lib/rubocop/cop/unnecessary_then.rb +++ b/lib/rubocop/cop/unnecessary_then.rb @@ -27,6 +27,12 @@ def multiline_if_then?(tokens, ix) end when :on_ignored_nl, :on_nl break + when :on_comment + break if t.text =~ /\n/ + when :on_sp + nil + else + then_found = false end end then_found diff --git a/spec/rubocop/cops/unnecessary_then_spec.rb b/spec/rubocop/cops/unnecessary_then_spec.rb index da1dfa746fc..a3b82b25484 100644 --- a/spec/rubocop/cops/unnecessary_then_spec.rb +++ b/spec/rubocop/cops/unnecessary_then_spec.rb @@ -11,14 +11,19 @@ module Cop it 'registers an offence for then in multiline if' do inspect_source(un_then, '', ['if cond then', - 'end']) - un_then.offences.map(&:message).sort.should == - ['Never use then for multi-line if/unless.'] + 'end', + "if cond then\t", + 'end', + "if cond then ", + 'end', + 'if cond then # bad', + 'end']) + un_then.offences.size.should == 4 end it 'accepts multiline if without then' do inspect_source(un_then, '', ['if cond', - 'end']) + 'end']) un_then.offences.map(&:message).sort.should == [] end @@ -27,18 +32,28 @@ module Cop un_then.offences.map(&:message).sort.should == [] end + it 'accepts table style if/then/elsif/ends' do + inspect_source(un_then, '', + ['if @io == $stdout then str << "$stdout"', + 'elsif @io == $stdin then str << "$stdin"', + 'elsif @io == $stderr then str << "$stderr"', + 'else str << @io.class.to_s', + 'end']) + un_then.offences.map(&:message).sort.should == [] + end + # unless it 'registers an offence for then in multiline unless' do inspect_source(un_then, '', ['unless cond then', - 'end']) + 'end']) un_then.offences.map(&:message).sort.should == ['Never use then for multi-line if/unless.'] end it 'accepts multiline unless without then' do inspect_source(un_then, '', ['unless cond', - 'end']) + 'end']) un_then.offences.map(&:message).sort.should == [] end From 7dabf3229c1c8b459c2d71b51e0ebe7b73995ed1 Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Tue, 1 Jan 2013 22:45:35 +0100 Subject: [PATCH 6/8] Added more checking of if/then/else, and so had to change some file names. --- lib/rubocop.rb | 2 +- lib/rubocop/cop/if_then_else.rb | 54 ++++++++++++++++ lib/rubocop/cop/unnecessary_then.rb | 42 ------------ spec/rubocop/cops/if_then_else_spec.rb | 74 ++++++++++++++++++++++ spec/rubocop/cops/unnecessary_then_spec.rb | 66 ------------------- 5 files changed, 129 insertions(+), 109 deletions(-) create mode 100644 lib/rubocop/cop/if_then_else.rb delete mode 100644 lib/rubocop/cop/unnecessary_then.rb create mode 100644 spec/rubocop/cops/if_then_else_spec.rb delete mode 100644 spec/rubocop/cops/unnecessary_then_spec.rb diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 17cf803120f..931e567c834 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -17,7 +17,7 @@ require 'rubocop/cop/numeric_literals' require 'rubocop/cop/align_parameters' require 'rubocop/cop/def_parentheses' -require 'rubocop/cop/unnecessary_then' +require 'rubocop/cop/if_then_else' require 'rubocop/report/report' require 'rubocop/report/plain_text' diff --git a/lib/rubocop/cop/if_then_else.rb b/lib/rubocop/cop/if_then_else.rb new file mode 100644 index 00000000000..91bf109bf4e --- /dev/null +++ b/lib/rubocop/cop/if_then_else.rb @@ -0,0 +1,54 @@ +# encoding: utf-8 + +module Rubocop + module Cop + class IfThenElse < Cop + ERROR_MESSAGE = + ['Never use then for multi-line if/unless.', + 'Favor the ternary operator (?:) over if/then/else/end constructs.', + 'Never use if x; Use the ternary operator instead.'] + + def inspect(file, source, tokens, sexp) + tokens.each_with_index do |t, ix| + if t.type == :on_kw && ['if', 'unless'].include?(t.text) + index = t.pos.lineno - 1 + error = case kind_of_if(tokens, ix + 1) + when :multiline_if_then then 0 + when :one_liner then 1 + when :semicolon then 2 + else nil + end + if error + add_offence(:convention, index, source[index], + ERROR_MESSAGE[error]) + end + end + end + end + + def kind_of_if(tokens, ix) + then_found = false + tokens[ix..-1].each do |t| + case t.type + when :on_kw + case t.text + when 'then' then then_found = true + when 'end' then return :one_liner + end + when :on_ignored_nl, :on_nl + break + when :on_semicolon + return :semicolon + when :on_comment + break if t.text =~ /\n/ + when :on_sp + nil + else + then_found = false + end + end + then_found ? :multiline_if_then : nil + end + end + end +end diff --git a/lib/rubocop/cop/unnecessary_then.rb b/lib/rubocop/cop/unnecessary_then.rb deleted file mode 100644 index baf57754509..00000000000 --- a/lib/rubocop/cop/unnecessary_then.rb +++ /dev/null @@ -1,42 +0,0 @@ -# encoding: utf-8 - -module Rubocop - module Cop - class UnnecessaryThen < Cop - ERROR_MESSAGE = 'Never use then for multi-line if/unless.' - - def inspect(file, source, tokens, sexp) - tokens.each_with_index do |t, ix| - if t.type == :on_kw && ['if', 'unless'].include?(t.text) - if multiline_if_then?(tokens, ix + 1) - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], ERROR_MESSAGE) - end - end - end - end - - def multiline_if_then?(tokens, ix) - then_found = false - tokens[ix..-1].each do |t| - case t.type - when :on_kw - case t.text - when 'then' then then_found = true - when 'end' then return false - end - when :on_ignored_nl, :on_nl - break - when :on_comment - break if t.text =~ /\n/ - when :on_sp - nil - else - then_found = false - end - end - then_found - end - end - end -end diff --git a/spec/rubocop/cops/if_then_else_spec.rb b/spec/rubocop/cops/if_then_else_spec.rb new file mode 100644 index 00000000000..383123e008f --- /dev/null +++ b/spec/rubocop/cops/if_then_else_spec.rb @@ -0,0 +1,74 @@ +# encoding: utf-8 + +require 'spec_helper' + +module Rubocop + module Cop + describe IfThenElse do + let (:if_then_else) { IfThenElse.new } + + # if + + it 'registers an offence for then in multiline if' do + inspect_source(if_then_else, '', ['if cond then', + 'end', + "if cond then\t", + 'end', + "if cond then ", + 'end', + 'if cond then # bad', + 'end']) + if_then_else.offences.size.should == 4 + end + + it 'accepts multiline if without then' do + inspect_source(if_then_else, '', ['if cond', + 'end']) + if_then_else.offences.map(&:message).sort.should == [] + end + + it 'registers an offence for one line if/then/end' do + inspect_source(if_then_else, '', ['if cond then run else dont end']) + if_then_else.offences.map(&:message).sort.should == + ['Favor the ternary operator (?:) over if/then/else/end constructs.'] + end + + it 'registers an offence for one line if/;/end' do + inspect_source(if_then_else, '', ['if cond; run else dont end']) + if_then_else.offences.map(&:message).sort.should == + ['Never use if x; Use the ternary operator instead.'] + end + + it 'accepts table style if/then/elsif/ends' do + inspect_source(if_then_else, '', + ['if @io == $stdout then str << "$stdout"', + 'elsif @io == $stdin then str << "$stdin"', + 'elsif @io == $stderr then str << "$stderr"', + 'else str << @io.class.to_s', + 'end']) + if_then_else.offences.map(&:message).sort.should == [] + end + + # unless + + it 'registers an offence for then in multiline unless' do + inspect_source(if_then_else, '', ['unless cond then', + 'end']) + if_then_else.offences.map(&:message).sort.should == + ['Never use then for multi-line if/unless.'] + end + + it 'accepts multiline unless without then' do + inspect_source(if_then_else, '', ['unless cond', + 'end']) + if_then_else.offences.map(&:message).sort.should == [] + end + + it 'registers an offence for one line unless/then/ends' do + inspect_source(if_then_else, '', ['unless cond then run end']) + if_then_else.offences.map(&:message).sort.should == + ['Favor the ternary operator (?:) over if/then/else/end constructs.'] + end + end + end +end diff --git a/spec/rubocop/cops/unnecessary_then_spec.rb b/spec/rubocop/cops/unnecessary_then_spec.rb deleted file mode 100644 index a3b82b25484..00000000000 --- a/spec/rubocop/cops/unnecessary_then_spec.rb +++ /dev/null @@ -1,66 +0,0 @@ -# encoding: utf-8 - -require 'spec_helper' - -module Rubocop - module Cop - describe UnnecessaryThen do - let (:un_then) { UnnecessaryThen.new } - - # if - - it 'registers an offence for then in multiline if' do - inspect_source(un_then, '', ['if cond then', - 'end', - "if cond then\t", - 'end', - "if cond then ", - 'end', - 'if cond then # bad', - 'end']) - un_then.offences.size.should == 4 - end - - it 'accepts multiline if without then' do - inspect_source(un_then, '', ['if cond', - 'end']) - un_then.offences.map(&:message).sort.should == [] - end - - it 'accepts one line if/then/ends' do - inspect_source(un_then, '', ['if cond then run end']) - un_then.offences.map(&:message).sort.should == [] - end - - it 'accepts table style if/then/elsif/ends' do - inspect_source(un_then, '', - ['if @io == $stdout then str << "$stdout"', - 'elsif @io == $stdin then str << "$stdin"', - 'elsif @io == $stderr then str << "$stderr"', - 'else str << @io.class.to_s', - 'end']) - un_then.offences.map(&:message).sort.should == [] - end - - # unless - - it 'registers an offence for then in multiline unless' do - inspect_source(un_then, '', ['unless cond then', - 'end']) - un_then.offences.map(&:message).sort.should == - ['Never use then for multi-line if/unless.'] - end - - it 'accepts multiline unless without then' do - inspect_source(un_then, '', ['unless cond', - 'end']) - un_then.offences.map(&:message).sort.should == [] - end - - it 'accepts one line unless/then/ends' do - inspect_source(un_then, '', ['unless cond then run end']) - un_then.offences.map(&:message).sort.should == [] - end - end - end -end From 3e070adfe2ca55dbdfc5caa9098377dbee3574f2 Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Wed, 2 Jan 2013 17:46:14 +0100 Subject: [PATCH 7/8] Updated line and line number reporting. Always counting line numbers from 1. Offending lines are not put in the Offence objects since they are not used. --- lib/rubocop/cop/align_parameters.rb | 3 +-- lib/rubocop/cop/cop.rb | 4 ++-- lib/rubocop/cop/def_parentheses.rb | 13 ++++--------- lib/rubocop/cop/empty_lines.rb | 3 +-- lib/rubocop/cop/encoding.rb | 2 +- lib/rubocop/cop/end_of_line.rb | 2 +- lib/rubocop/cop/hash_syntax.rb | 4 ++-- lib/rubocop/cop/if_then_else.rb | 4 +--- lib/rubocop/cop/indentation.rb | 3 +-- lib/rubocop/cop/line_length.rb | 2 +- lib/rubocop/cop/numeric_literals.rb | 3 +-- lib/rubocop/cop/offence.rb | 5 ++--- lib/rubocop/cop/space_after_comma_etc.rb | 4 +--- lib/rubocop/cop/surrounding_space.rb | 14 ++++---------- lib/rubocop/cop/tab.rb | 2 +- lib/rubocop/cop/trailing_whitespace.rb | 2 +- lib/rubocop/report/emacs_style.rb | 2 +- spec/rubocop/cli_spec.rb | 2 +- spec/rubocop/cops/align_parameters_spec.rb | 4 ++-- spec/rubocop/cops/cop_spec.rb | 4 ++-- spec/rubocop/cops/empty_lines_spec.rb | 3 +-- spec/rubocop/cops/offence_spec.rb | 5 ++--- spec/rubocop/reports/emacs_style_spec.rb | 4 ++-- spec/rubocop/reports/report_spec.rb | 2 +- 24 files changed, 37 insertions(+), 59 deletions(-) diff --git a/lib/rubocop/cop/align_parameters.rb b/lib/rubocop/cop/align_parameters.rb index a2eecfd87d2..8df6ffad2e5 100644 --- a/lib/rubocop/cop/align_parameters.rb +++ b/lib/rubocop/cop/align_parameters.rb @@ -21,8 +21,7 @@ def inspect(file, source, tokens, sexp) pos = position_of(arg) or next # Give up if no position found. if pos.lineno != pos_of_1st_arg.lineno if pos.column != pos_of_1st_arg.column - index = pos.lineno - 1 - add_offence(:convention, index, source[index], ERROR_MESSAGE) + add_offence(:convention, pos.lineno, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/cop.rb b/lib/rubocop/cop/cop.rb index bfb671a220e..56ac41af067 100644 --- a/lib/rubocop/cop/cop.rb +++ b/lib/rubocop/cop/cop.rb @@ -63,8 +63,8 @@ def has_report? !@offences.empty? end - def add_offence(file, line_number, line, message) - @offences << Offence.new(file, line_number, line, message) + def add_offence(file, line_number, message) + @offences << Offence.new(file, line_number, message) end private diff --git a/lib/rubocop/cop/def_parentheses.rb b/lib/rubocop/cop/def_parentheses.rb index a3c70b63d8a..3c566e90b67 100644 --- a/lib/rubocop/cop/def_parentheses.rb +++ b/lib/rubocop/cop/def_parentheses.rb @@ -13,7 +13,9 @@ def inspect(file, source, tokens, sexp) pos = def_sexp[1][-1] case def_sexp[2][0] when :params - add(pos, source, ERROR_MESSAGE[0]) if def_sexp[2] != EMPTY_PARAMS + if def_sexp[2] != EMPTY_PARAMS + add_offence(:convention, pos.lineno, ERROR_MESSAGE[0]) + end when :paren if def_sexp[2][1] == EMPTY_PARAMS method_name_ix = tokens.index { |t| t.pos == pos } @@ -25,19 +27,12 @@ def inspect(file, source, tokens, sexp) if first_body_token.pos.lineno > pos.lineno # Only report offence if there's a line break after # the empty parens. - add(pos, source, ERROR_MESSAGE[1]) + add_offence(:convention, pos.lineno, ERROR_MESSAGE[1]) end end end end end - - private - - def add(pos, source, message) - index = pos.lineno - 1 - add_offence(:convention, index, source[index], message) - end end end end diff --git a/lib/rubocop/cop/empty_lines.rb b/lib/rubocop/cop/empty_lines.rb index 594e0c10417..522c0885708 100644 --- a/lib/rubocop/cop/empty_lines.rb +++ b/lib/rubocop/cop/empty_lines.rb @@ -15,8 +15,7 @@ def inspect(file, source, tokens, sexp) defs[1..-1].each do |child| next_row_ix = child[1][-1].lineno - 1 if source[current_row_ix..next_row_ix].grep(/^[ \t]*$/).empty? - add_offence(:convention, next_row_ix, source[next_row_ix], - ERROR_MESSAGE) + add_offence(:convention, next_row_ix + 1, ERROR_MESSAGE) end current_row_ix = next_row_ix end diff --git a/lib/rubocop/cop/encoding.rb b/lib/rubocop/cop/encoding.rb index ec9e000f216..4529bd8f7ac 100644 --- a/lib/rubocop/cop/encoding.rb +++ b/lib/rubocop/cop/encoding.rb @@ -9,7 +9,7 @@ class Encoding < Cop def inspect(file, source, tokens, sexp) unless source[0] =~ /#.*coding: (UTF|utf)-8/ message = sprintf(ERROR_MESSAGE) - add_offence(:convention, 0, 0, message) + add_offence(:convention, 1, message) end end end diff --git a/lib/rubocop/cop/end_of_line.rb b/lib/rubocop/cop/end_of_line.rb index 6c5863d6ae4..33d14cce1a4 100644 --- a/lib/rubocop/cop/end_of_line.rb +++ b/lib/rubocop/cop/end_of_line.rb @@ -8,7 +8,7 @@ class EndOfLine < Cop def inspect(file, source, tokens, sexp) source.each_with_index do |line, index| if line =~ /\r$/ - add_offence(:convention, index, line, ERROR_MESSAGE) + add_offence(:convention, index + 1, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/hash_syntax.rb b/lib/rubocop/cop/hash_syntax.rb index 1e2585ac650..91b296c39f5 100644 --- a/lib/rubocop/cop/hash_syntax.rb +++ b/lib/rubocop/cop/hash_syntax.rb @@ -16,8 +16,8 @@ def inspect(file, source, tokens, sexp) end each(:assoc_new, sexp) do |assoc_new| if assoc_new[1][0] == :symbol_literal - index = assoc_new[1][1][1][-1].lineno - 1 - add_offence(:convention, index, source[index], ERROR_MESSAGE) + add_offence(:convention, assoc_new[1][1][1][-1].lineno, + ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/if_then_else.rb b/lib/rubocop/cop/if_then_else.rb index 91bf109bf4e..3c0cb100d61 100644 --- a/lib/rubocop/cop/if_then_else.rb +++ b/lib/rubocop/cop/if_then_else.rb @@ -11,7 +11,6 @@ class IfThenElse < Cop def inspect(file, source, tokens, sexp) tokens.each_with_index do |t, ix| if t.type == :on_kw && ['if', 'unless'].include?(t.text) - index = t.pos.lineno - 1 error = case kind_of_if(tokens, ix + 1) when :multiline_if_then then 0 when :one_liner then 1 @@ -19,8 +18,7 @@ def inspect(file, source, tokens, sexp) else nil end if error - add_offence(:convention, index, source[index], - ERROR_MESSAGE[error]) + add_offence(:convention, t.pos.lineno, ERROR_MESSAGE[error]) end end end diff --git a/lib/rubocop/cop/indentation.rb b/lib/rubocop/cop/indentation.rb index 6c2fe612943..8006aa88440 100644 --- a/lib/rubocop/cop/indentation.rb +++ b/lib/rubocop/cop/indentation.rb @@ -11,8 +11,7 @@ def inspect(file, source, tokens, sexp) each_when(sexp) do |case_ix| when_pos = when_tokens.shift.pos if when_pos.column != case_tokens[case_ix].pos.column - index = when_pos.lineno - 1 - add_offence(:convention, index, source[index], ERROR_MESSAGE) + add_offence(:convention, when_pos.lineno, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/line_length.rb b/lib/rubocop/cop/line_length.rb index 7e3916f04fb..797779042e7 100644 --- a/lib/rubocop/cop/line_length.rb +++ b/lib/rubocop/cop/line_length.rb @@ -10,7 +10,7 @@ def inspect(file, source, tokens, sexp) source.each_with_index do |line, index| if line.length > MAX_LINE_LENGTH message = sprintf(ERROR_MESSAGE, line.length, MAX_LINE_LENGTH) - add_offence(:convention, index, line, message) + add_offence(:convention, index + 1, message) end end end diff --git a/lib/rubocop/cop/numeric_literals.rb b/lib/rubocop/cop/numeric_literals.rb index 2f29417c3e3..bfe9b464004 100644 --- a/lib/rubocop/cop/numeric_literals.rb +++ b/lib/rubocop/cop/numeric_literals.rb @@ -10,8 +10,7 @@ def inspect(file, source, tokens, sexp) tokens.each do |t| if [:on_int, :on_float].include?(t.type) && t.text.split('.').grep(/\d{6}/).any? - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], ERROR_MESSAGE) + add_offence(:convention, t.pos.lineno, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/offence.rb b/lib/rubocop/cop/offence.rb index b021e4bed9a..618369bc110 100644 --- a/lib/rubocop/cop/offence.rb +++ b/lib/rubocop/cop/offence.rb @@ -3,14 +3,13 @@ module Rubocop module Cop class Offence - attr_accessor :severity, :line_number, :line, :message + attr_accessor :severity, :line_number, :message SEVERITIES = [:refactor, :convention, :warning, :error, :fatal] - def initialize(severity, line_number, line, message) + def initialize(severity, line_number, message) @severity = severity @line_number = line_number - @line = line @message = message end diff --git a/lib/rubocop/cop/space_after_comma_etc.rb b/lib/rubocop/cop/space_after_comma_etc.rb index 0e3bec42d5a..86595aeb5c0 100644 --- a/lib/rubocop/cop/space_after_comma_etc.rb +++ b/lib/rubocop/cop/space_after_comma_etc.rb @@ -16,9 +16,7 @@ def inspect(file, source, tokens, sexp) end if kind and not [:on_sp, :on_ignored_nl].include?(tokens[ix + 1].type) - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], - ERROR_MESSAGE % kind) + add_offence(:convention, t.pos.lineno, ERROR_MESSAGE % kind) end end end diff --git a/lib/rubocop/cop/surrounding_space.rb b/lib/rubocop/cop/surrounding_space.rb index d5d0300394c..3162c325c2a 100644 --- a/lib/rubocop/cop/surrounding_space.rb +++ b/lib/rubocop/cop/surrounding_space.rb @@ -14,21 +14,17 @@ def inspect(file, source, tokens, sexp) when :on_op unless surrounded_by_whitespace?(tokens[ix - 1, 3]) unless ok_without_spaces?(grammar_path) - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], + add_offence(:convention, t.pos.lineno, ERROR_MESSAGE + "operator '#{t.text}'.") end end when :on_lbrace unless surrounded_by_whitespace?(tokens[ix - 1, 3]) - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], - ERROR_MESSAGE + "'{'.") + add_offence(:convention, t.pos.lineno, ERROR_MESSAGE + "'{'.") end when :on_rbrace unless whitespace?(tokens[ix - 1]) - index = t.pos.lineno - 1 - add_offence(:convention, index, source[index], + add_offence(:convention, t.pos.lineno, "Space missing to the left of '}'.") end end @@ -53,7 +49,6 @@ def inspect(file, source, tokens, sexp) (whitespace?(prev) || whitespace?(nxt)) end if offence_detected - index = t.pos.lineno - 1 kind = case t.type when :on_lparen, :on_rparen 'inside parentheses' @@ -62,8 +57,7 @@ def inspect(file, source, tokens, sexp) when :on_op "around operator #{t.text}" end - add_offence(:convention, index, source[index], - "Space #{kind} detected.") + add_offence(:convention, t.pos.lineno, "Space #{kind} detected.") end end end diff --git a/lib/rubocop/cop/tab.rb b/lib/rubocop/cop/tab.rb index 8510a4ba564..ae58e3d43aa 100644 --- a/lib/rubocop/cop/tab.rb +++ b/lib/rubocop/cop/tab.rb @@ -8,7 +8,7 @@ class Tab < Cop def inspect(file, source, tokens, sexp) source.each_with_index do |line, index| if line =~ /^ *\t/ - add_offence(:convention, index, line, ERROR_MESSAGE) + add_offence(:convention, index + 1, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/cop/trailing_whitespace.rb b/lib/rubocop/cop/trailing_whitespace.rb index 4124c456ee6..fca1b72a0d9 100644 --- a/lib/rubocop/cop/trailing_whitespace.rb +++ b/lib/rubocop/cop/trailing_whitespace.rb @@ -8,7 +8,7 @@ class TrailingWhitespace < Cop def inspect(file, source, tokens, sexp) source.each_with_index do |line, index| if line =~ /.*[ \t]+$/ - add_offence(:convention, index, line, ERROR_MESSAGE) + add_offence(:convention, index + 1, ERROR_MESSAGE) end end end diff --git a/lib/rubocop/report/emacs_style.rb b/lib/rubocop/report/emacs_style.rb index 76f680114d7..dd4efb3b8a9 100644 --- a/lib/rubocop/report/emacs_style.rb +++ b/lib/rubocop/report/emacs_style.rb @@ -7,7 +7,7 @@ class EmacsStyle < PlainText # Generates a string representation of the report def generate report = entries.map do |e| - "#@filename:#{e.line_number + 1}: #{e.encode_severity}: #{e.message}" + "#@filename:#{e.line_number}: #{e.encode_severity}: #{e.message}" end report.join("\n") end diff --git a/spec/rubocop/cli_spec.rb b/spec/rubocop/cli_spec.rb index 9a113d5d26e..07d797a6dd2 100644 --- a/spec/rubocop/cli_spec.rb +++ b/spec/rubocop/cli_spec.rb @@ -39,7 +39,7 @@ module Report begin cli.run(['example.rb']).should == 1 $stdout.string.should == ['== example.rb ==', - 'C: 1: Trailing whitespace detected.', + 'C: 2: Trailing whitespace detected.', '', '1 files inspected, 1 offences detected', ''].join("\n") diff --git a/spec/rubocop/cops/align_parameters_spec.rb b/spec/rubocop/cops/align_parameters_spec.rb index e18667be551..fca6d29e39d 100644 --- a/spec/rubocop/cops/align_parameters_spec.rb +++ b/spec/rubocop/cops/align_parameters_spec.rb @@ -38,7 +38,7 @@ module Cop it "doesn't get confused by a symbol argument" do inspect_source(align, '', - ['add_offence(:convention, index, source[index],', + ['add_offence(:convention, index,', ' ERROR_MESSAGE % kind)']) align.offences.map(&:message).should == [] end @@ -54,7 +54,7 @@ module Cop 'func3(*a)', ]) align.offences.map(&:to_s).should == - ['C: 4: Align the parameters of a method call if they span more ' + + ['C: 5: Align the parameters of a method call if they span more ' + 'than one line.'] end diff --git a/spec/rubocop/cops/cop_spec.rb b/spec/rubocop/cops/cop_spec.rb index dca49cad2cf..5e04c6999e2 100644 --- a/spec/rubocop/cops/cop_spec.rb +++ b/spec/rubocop/cops/cop_spec.rb @@ -16,13 +16,13 @@ module Cop end it 'keeps track of offences' do - cop.add_offence('file', 0, 'line', 'message') + cop.add_offence('file', 1, 'message') cop.offences.size.should == 1 end it 'will report registered offences' do - cop.add_offence('file', 0, 'line', 'message') + cop.add_offence('file', 1, 'message') cop.has_report?.should be_true end diff --git a/spec/rubocop/cops/empty_lines_spec.rb b/spec/rubocop/cops/empty_lines_spec.rb index 3184a8abb04..33b477284f7 100644 --- a/spec/rubocop/cops/empty_lines_spec.rb +++ b/spec/rubocop/cops/empty_lines_spec.rb @@ -22,8 +22,7 @@ module Cop ' end', 'end']) empty_lines.offences.size.should == 2 - empty_lines.offences.map(&:line).sort.should == [' def o', - ' def p'] + empty_lines.offences.map(&:line_number).sort.should == [7, 11] end # Only one def, so rule about empty line *between* defs does not diff --git a/spec/rubocop/cops/offence_spec.rb b/spec/rubocop/cops/offence_spec.rb index dd5d2ebeab7..51fc6b909ae 100644 --- a/spec/rubocop/cops/offence_spec.rb +++ b/spec/rubocop/cops/offence_spec.rb @@ -6,16 +6,15 @@ module Rubocop module Cop describe Offence do it 'has a few required attributes' do - offence = Offence.new(:convention, 1, 'line', 'message') + offence = Offence.new(:convention, 1, 'message') offence.severity.should == :convention offence.line_number.should == 1 - offence.line.should == 'line' offence.message.should == 'message' end it 'overrides #to_s' do - offence = Offence.new(:convention, 1, 'line', 'message') + offence = Offence.new(:convention, 1, 'message') offence.to_s.should == 'C: 1: message' end diff --git a/spec/rubocop/reports/emacs_style_spec.rb b/spec/rubocop/reports/emacs_style_spec.rb index dbcd6b8a667..5745d21a300 100644 --- a/spec/rubocop/reports/emacs_style_spec.rb +++ b/spec/rubocop/reports/emacs_style_spec.rb @@ -10,8 +10,8 @@ module Report it 'displays parsable text' do cop = Cop::Cop.new - cop.add_offence(:convention, 0, 'line', 'message 1') - cop.add_offence(:fatal, 10, 'line', 'message 2') + cop.add_offence(:convention, 1, 'message 1') + cop.add_offence(:fatal, 11, 'message 2') emacs_style << cop diff --git a/spec/rubocop/reports/report_spec.rb b/spec/rubocop/reports/report_spec.rb index fd75e299223..a0321e855ba 100644 --- a/spec/rubocop/reports/report_spec.rb +++ b/spec/rubocop/reports/report_spec.rb @@ -17,7 +17,7 @@ module Report it 'keeps track of offences' do cop = Cop::Cop.new - cop.add_offence(:convention, 0, 'line', 'message') + cop.add_offence(:convention, 1, 'message') report << cop From fbb3d8281f97986e2d9687269d8ed6e582087f0a Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Wed, 2 Jan 2013 19:24:53 +0100 Subject: [PATCH 8/8] A bit of refactoring in IfThenElse. --- lib/rubocop/cop/if_then_else.rb | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/lib/rubocop/cop/if_then_else.rb b/lib/rubocop/cop/if_then_else.rb index 3c0cb100d61..57451ae62f2 100644 --- a/lib/rubocop/cop/if_then_else.rb +++ b/lib/rubocop/cop/if_then_else.rb @@ -3,23 +3,20 @@ module Rubocop module Cop class IfThenElse < Cop - ERROR_MESSAGE = - ['Never use then for multi-line if/unless.', - 'Favor the ternary operator (?:) over if/then/else/end constructs.', - 'Never use if x; Use the ternary operator instead.'] + ERROR_MESSAGE = { + multiline_if_then: + 'Never use then for multi-line if/unless.', + one_liner: + 'Favor the ternary operator (?:) over if/then/else/end constructs.', + semicolon: + 'Never use if x; Use the ternary operator instead.' + } def inspect(file, source, tokens, sexp) tokens.each_with_index do |t, ix| if t.type == :on_kw && ['if', 'unless'].include?(t.text) - error = case kind_of_if(tokens, ix + 1) - when :multiline_if_then then 0 - when :one_liner then 1 - when :semicolon then 2 - else nil - end - if error - add_offence(:convention, t.pos.lineno, ERROR_MESSAGE[error]) - end + error = ERROR_MESSAGE[kind_of_if(tokens, ix + 1)] + add_offence(:convention, t.pos.lineno, error) if error end end end