Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

0.37.1 Performance/RedundantMerge shouldn't be applied when merge! result might be used #2831

Closed
mikestok opened this issue Feb 11, 2016 · 1 comment

Comments

@mikestok
Copy link

Consider an rspec spec file:

describe 'problem' do
  let(:hash) { { foo: 'bar' } }
  let(:merged) { hash.merge!(baz: 'plugh') }

  it 'should have correct contents' do
    expect(merged).to eq(foo: 'bar', baz: 'plugh')
  end
end

the auto-correct in 0.37.1 will correct that to use let(:merged) { hash[:baz] = 'plugh' }. The result of the assignment is the string plugh, but the result of the merge! was the hash (now modified).

Maybe Performance/RedundantMerge shouldn't apply if the merge! is the last thing in a block.

21:42 $ bundle exec rubocop --auto-correct --debug --config /dev/null bug_spec.rb
configuration from /dev/null
Default configuration from /Users/mike/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.37.1/config/default.yml
Inheriting configuration from /Users/mike/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.37.1/config/enabled.yml
Inheriting configuration from /Users/mike/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.37.1/config/disabled.yml
Inspecting 1 file
Scanning /Users/mike/Projects/influitive/hub/bug_spec.rb
C

Offenses:

bug_spec.rb:3:18: C: [Corrected] Performance/RedundantMerge: Use hash[:baz] = 'plugh' instead of hash.merge!(baz: 'plugh').
  let(:merged) { hash.merge!(baz: 'plugh') }
                 ^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
Finished in 0.3582553739834111 seconds
$ cat bug_spec.rb
describe 'problem' do
  let(:hash) { { foo: 'bar' } }
  let(:merged) { hash[:baz] = 'plugh' }

  it 'should have correct contents' do
    expect(merged).to eq(foo: 'bar', baz: 'plugh')
  end
end
@alexdowad
Copy link
Contributor

Just pushed a fix to my open PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants