From 80d8ca670c235107a77cc821a824392b622d3aec Mon Sep 17 00:00:00 2001 From: Ryan Rosenblum Date: Thu, 7 May 2015 09:23:25 -0400 Subject: [PATCH] [Fix #1868] Only register an offense in Count for select with block or &:something format --- CHANGELOG.md | 1 + lib/rubocop/cop/performance/count.rb | 58 ++++++++++++++-------- spec/rubocop/cop/performance/count_spec.rb | 35 ++++++++++++- 3 files changed, 70 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf5c66d7f8af..0c46ce1e0fe7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bugs fixed * Don't count required keyword args when specifying `CountKeywordArgs: false` for `ParameterLists`. ([@sumeet][]) +* [#1868](https://github.com/bbatsov/rubocop/issues/1868): Do not register an offense in Performance/Count when `select` is called with symbols or strings as the parameters. ([@rrosenblum][]) ## 0.31.0 (05/05/2015) diff --git a/lib/rubocop/cop/performance/count.rb b/lib/rubocop/cop/performance/count.rb index 05afa0f15573..e7e6e66f09e8 100644 --- a/lib/rubocop/cop/performance/count.rb +++ b/lib/rubocop/cop/performance/count.rb @@ -14,12 +14,15 @@ module Performance # [1, 2, 3].reject { |e| e > 2 }.length # [1, 2, 3].select { |e| e > 2 }.count { |e| e.odd? } # [1, 2, 3].reject { |e| e > 2 }.count { |e| e.even? } + # array.select(&:value).count # # # good # [1, 2, 3].count { |e| e > 2 } # [1, 2, 3].count { |e| e < 2 } # [1, 2, 3].count { |e| e > 2 && e.odd? } # [1, 2, 3].count { |e| e < 2 && e.even? } + # Model.select('field AS field_one').count + # Model.select(:value).count class Count < Cop MSG = 'Use `count` instead of `%s...%s`.' @@ -27,56 +30,67 @@ class Count < Cop COUNTERS = [:count, :length, :size] def on_send(node) - expression, first_method, second_method, third_method = parse(node) + expression, selector, params, counter = parse(node) + return unless COUNTERS.include?(counter) - return unless COUNTERS.include?(third_method) - - begin_pos = if SELECTORS.include?(first_method) - return if second_method.is_a?(Symbol) - expression.loc.selector.begin_pos + begin_pos = if selector.is_a?(Symbol) + expression.parent.loc.selector.begin_pos if expression else - return unless SELECTORS.include?(second_method) - expression.parent.loc.selector.begin_pos + _enumerable, selector, params = *expression + + if contains_selector?(expression) + expression.loc.selector.begin_pos + end end + return unless SELECTORS.include?(selector) + return if params && !params.block_pass_type? return if node.parent && node.parent.block_type? + begin_pos ||= node.loc.expression.begin_pos + range = Parser::Source::Range.new(node.loc.expression.source_buffer, begin_pos, node.loc.expression.end_pos) - add_offense(node, range, - format(MSG, first_method || second_method, third_method)) + add_offense(node, range, format(MSG, selector, counter)) end def autocorrect(node) - expression, first_method, second_method, = parse(node) + expression, selector, = parse(node) - return if first_method == :reject || second_method == :reject + selector_loc = if selector.is_a?(Symbol) + expression.parent.loc.selector + else + _enumerable, selector, = *expression - selector = if SELECTORS.include?(first_method) - expression.loc.selector - else - expression.parent.loc.selector - end + if expression.loc.respond_to?(:selector) + expression.loc.selector + end + end + + return if selector == :reject lambda do |corrector| range = Parser::Source::Range.new(node.loc.expression.source_buffer, node.loc.dot.begin_pos, node.loc.expression.end_pos) corrector.remove(range) - corrector.replace(selector, 'count') + corrector.replace(selector_loc, 'count') end end private def parse(node) - left, third_method = *node - expression, second_method = *left - _enumerable, first_method = *expression + left, counter = *node + expression, selector, params = *left + + [expression, selector, params, counter] + end - [expression, first_method, second_method, third_method] + def contains_selector?(node) + node.respond_to?(:loc) && node.loc.respond_to?(:selector) end end end diff --git a/spec/rubocop/cop/performance/count_spec.rb b/spec/rubocop/cop/performance/count_spec.rb index 756729844491..9310f306db7e 100644 --- a/spec/rubocop/cop/performance/count_spec.rb +++ b/spec/rubocop/cop/performance/count_spec.rb @@ -48,14 +48,14 @@ .to eq(["Use `count` instead of `#{selector}...count`."]) end - it "allows usage of #{selector}...count with a block" do + it "allows usage of #{selector}...count with a block on an array" do inspect_source(cop, "[1, 2, 3].#{selector} { |e| e.odd? }.count { |e| e > 2 }") expect(cop.messages).to be_empty end - it "allows usage of #{selector}...count with a block" do + it "allows usage of #{selector}...count with a block on a hash" do source = "{a: 1, b: 2}.#{selector} { |e| e == :a }.count { |e| e > 2 }" inspect_source(cop, source) @@ -102,6 +102,37 @@ it_behaves_like('selectors', 'select') it_behaves_like('selectors', 'reject') + it 'registers an offense for select(&:something).count' do + inspect_source(cop, 'foo.select(&:something).count') + + expect(cop.messages.size).to eq(1) + end + + it 'allows usage of select with a string' do + inspect_source(cop, "Model.select('field AS field_one').count") + + expect(cop.messages).to be_empty + end + + it 'allows usage of select with multiple strings' do + source = "Model.select('field AS field_one', 'other AS field_two').count" + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows usage of select with a symbol' do + inspect_source(cop, 'Model.select(:field).count') + + expect(cop.messages).to be_empty + end + + it 'allows usage of select with multiple symbols' do + inspect_source(cop, 'Model.select(:field, :other_field).count') + + expect(cop.messages).to be_empty + end + it 'allows usage of another method with size' do inspect_source(cop, '[1, 2, 3].map { |e| e + 1 }.size')