From 269ea0bda61dc3c98183dd1aebc31722eeef6666 Mon Sep 17 00:00:00 2001 From: Tejas Bubane Date: Fri, 23 Sep 2016 18:43:40 +0530 Subject: [PATCH] [Fix #3512] Change error message of `Lint/UnneededSplatExpansion` for array (#3526) In case of array splat for method parameter, the default message was ambiguous. `Unnecessary splat expansion.` made people just remove the splat expansion and pass the array as parameter which is obviously not the same as passing splat arguments. Change the message to `Pass array contents as separate arguments.` to the message in such cases to make the intension more clear. --- CHANGELOG.md | 4 ++ .../cop/lint/unneeded_splat_expansion.rb | 16 +++++- .../cop/lint/unneeded_splat_expansion_spec.rb | 57 ++++++++++++------- 3 files changed, 56 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a85d35f7444b..78dd1e0ba447 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ * [#3513](https://github.com/bbatsov/rubocop/pull/3513): Fix false positive in `Style/TernaryParentheses` for a ternary with ranges. ([@dreyks][]) * [#3520](https://github.com/bbatsov/rubocop/issues/3520): Fix regression causing `Lint/AssignmentInCondition` false positive. ([@savef][]) +### Changes + +* [#3512](https://github.com/bbatsov/rubocop/issues/3512): Change error message of `Lint/UnneededSplatExpansion` for array in method parameters. ([@tejasbubane][]) + ## 0.43.0 (2016-09-19) ### New features diff --git a/lib/rubocop/cop/lint/unneeded_splat_expansion.rb b/lib/rubocop/cop/lint/unneeded_splat_expansion.rb index 6f7681f9f227..3bb7f51b014d 100644 --- a/lib/rubocop/cop/lint/unneeded_splat_expansion.rb +++ b/lib/rubocop/cop/lint/unneeded_splat_expansion.rb @@ -47,6 +47,7 @@ module Lint # end class UnneededSplatExpansion < Cop MSG = 'Unnecessary splat expansion.'.freeze + ARRAY_PARAM_MSG = 'Pass array contents as separate arguments.'.freeze PERCENT_W = '%w'.freeze PERCENT_CAPITAL_W = '%W'.freeze PERCENT_I = '%i'.freeze @@ -63,7 +64,12 @@ def on_splat(node) if object.send_type? return unless ASSIGNMENT_TYPES.include?(node.parent.parent.type) end - add_offense(node, :expression) + + if array_splat?(node) && method_argument?(node) + add_offense(node, :expression, ARRAY_PARAM_MSG) + else + add_offense(node, :expression) + end end end @@ -85,6 +91,14 @@ def autocorrect(node) end end + def array_splat?(node) + node.children.first.array_type? + end + + def method_argument?(node) + node.parent.send_type? + end + def unneeded_brackets?(node) parent = node.parent diff --git a/spec/rubocop/cop/lint/unneeded_splat_expansion_spec.rb b/spec/rubocop/cop/lint/unneeded_splat_expansion_spec.rb index 74faa6a9b9cf..64b604319dbe 100644 --- a/spec/rubocop/cop/lint/unneeded_splat_expansion_spec.rb +++ b/spec/rubocop/cop/lint/unneeded_splat_expansion_spec.rb @@ -5,6 +5,8 @@ describe RuboCop::Cop::Lint::UnneededSplatExpansion do subject(:cop) { described_class.new } + let(:message) { 'Unnecessary splat expansion.' } + let(:array_param_message) { 'Pass array contents as separate arguments.' } it 'allows assigning to a splat' do inspect_source(cop, '*, rhs = *node') @@ -43,29 +45,44 @@ expect(cop.offenses).to be_empty end - shared_examples 'splat expansion' do |literal| - context 'expansion of method parameters' do - it 'registers an offense for expansion of parameters' do + shared_examples 'splat literal assignment' do |literal| + it 'registers an offense for ' do + inspect_source(cop, "a = *#{literal}") + + expect(cop.messages).to eq([message]) + expect(cop.highlights).to eq(["*#{literal}"]) + end + end + + shared_examples 'array splat expansion' do |literal| + context 'method parameters' do + it 'registers an offense' do inspect_source(cop, "array.push(*#{literal})") - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([array_param_message]) expect(cop.highlights).to eq(["*#{literal}"]) end end - context 'assignment to splat expansion of literals' do - it 'registers an offense for ' do - inspect_source(cop, "a = *#{literal}") + it_behaves_like 'splat literal assignment', literal + end - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + shared_examples 'splat expansion' do |literal| + context 'method parameters' do + it 'registers an offense' do + inspect_source(cop, "array.push(*#{literal})") + + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(["*#{literal}"]) end end + + it_behaves_like 'splat literal assignment', literal end - it_behaves_like 'splat expansion', '[1, 2, 3]' - it_behaves_like 'splat expansion', '%w(one two three)' - it_behaves_like 'splat expansion', '%W(one #{two} three)' + it_behaves_like 'array splat expansion', '[1, 2, 3]' + it_behaves_like 'array splat expansion', '%w(one two three)' + it_behaves_like 'array splat expansion', '%W(one #{two} three)' it_behaves_like 'splat expansion', "'a'" it_behaves_like 'splat expansion', '"#{a}"' it_behaves_like 'splat expansion', '1' @@ -75,7 +92,7 @@ it 'registers an offense for an array using a constructor' do inspect_source(cop, 'a = *Array.new(3) { 42 }') - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(['*Array.new(3) { 42 }']) end end @@ -87,7 +104,7 @@ ' bar', 'end']) - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(['*[first, second]']) end @@ -97,7 +114,7 @@ ' bar', 'end']) - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(['*%w(first second)']) end @@ -107,7 +124,7 @@ ' bar', 'end']) - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(['*%W(#{first} second)']) end @@ -138,7 +155,7 @@ ' bar', 'end']) - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([message]) expect(cop.highlights).to eq(['*[First, Second]']) end @@ -284,8 +301,8 @@ end context 'ruby >= 2.0', :ruby20 do - it_behaves_like 'splat expansion', '%i(first second)' - it_behaves_like 'splat expansion', '%I(first second #{third})' + it_behaves_like 'array splat expansion', '%i(first second)' + it_behaves_like 'array splat expansion', '%I(first second #{third})' context 'arrays being expanded with %i variants using splat expansion' do it 'registers an offense for an array literal being expanded in a ' \ @@ -306,14 +323,14 @@ it 'registers an offense for an array literal %i' do inspect_source(cop, 'array.push(*%i(first second))') - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([array_param_message]) expect(cop.highlights).to eq(['*%i(first second)']) end it 'registers an offense for an array literal %I' do inspect_source(cop, 'array.push(*%I(#{first} second))') - expect(cop.messages).to eq(['Unnecessary splat expansion.']) + expect(cop.messages).to eq([array_param_message]) expect(cop.highlights).to eq(['*%I(#{first} second)']) end end