Add new ArrayPushSingle cop to replace a.push(x) with a << x
#433
Conversation
amomchilov
force-pushed
the
ArrayPushSingle
branch
from
8a2164d
to
58e20b8
Compare
| Performance/ArrayPushSingle: | ||
| Description: 'Use `array << x` instead of `array.push(x)`.' | ||
| Reference: 'https://github.com/fastruby/fast-ruby#ancestorsinclude-vs--code' | ||
| Enabled: true |
There was a problem hiding this comment.
Should it be true, false or pending?
There was a problem hiding this comment.
The new cop generator should output pending. On the next major release all pending cops are enabled in bulk according to the docs.
| @@ -48,7 +48,7 @@ def replacement(regexp_node) | |||
| stack = [] | |||
| chars = regexp_content.chars.each_with_object([]) do |char, strings| | |||
| if stack.empty? && char == '\\' | |||
| stack.push(char) | |||
| stack.push(char) # rubocop:disable Performance/ArrayPushSingle | |||
There was a problem hiding this comment.
Keeping push is nicer here, to match pop below
| ^^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `push`. | ||
| RUBY | ||
|
|
||
| # gross. TODO: make a configuration option? |
There was a problem hiding this comment.
What's the best way to handle this?
There was a problem hiding this comment.
I'd just ignore it. &.<< looks horrible
amomchilov
force-pushed
the
ArrayPushSingle
branch
from
58e20b8
to
fca972f
Compare
| message = format(MSG, current: method_name) | ||
|
|
||
| add_offense(node, message: message) do |corrector| | ||
| corrector.replace(node, "#{receiver.source} << #{element.source}") |
There was a problem hiding this comment.
there are some edge cases that this won't handle correctly e.g.:
# original
[].push(1 && 2) # result is [2]
# corrected
[] << 1 && 2 # result is 2
# original
[].push 1 << 2 # result is [4]
# corrected
[] << 1 << 2 # result is [1,2]
# original
[].push(1) + [].push(2) # result is [1,2]
# corrected
[] << 1 + [] << 2
TypeError: Array can't be coerced into IntegerThere was a problem hiding this comment.
Hmm does Rubocop have any utilities for detecting the precedence of a subexpr, relative to the other parts of the expression it's in?
| it 'registers an offense and corrects when using `push` with a single element' do | ||
| expect_offense(<<~RUBY) | ||
| array.push(element) | ||
| ^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `push`. |
There was a problem hiding this comment.
should probably add a test for append as well
| ^^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `push`. | ||
| RUBY | ||
|
|
||
| # gross. TODO: make a configuration option? |
There was a problem hiding this comment.
I'd just ignore it. &.<< looks horrible
|
Is this related to rubocop/rubocop#12053 ? |
|
@ydakuka I hadn't seen that, but I guess it's pretty similar! I think two separate cops would be good here. One to turn |
Closes #431
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.