[Fix #5054] Lint/UnneededSplatExpansion: Array.new inside literal #5071
Conversation
akhramov
changed the title
UnneededSplatExpansion: Array.new inside literal
akhramov
force-pushed
the
fix/unneeded-splat-expansion-array-new-in-literal
branch
from
cc6f8f3 to
78fe952
Compare
| grandparent.array_type? && grandparent.children.count > 1 | ||
| end | ||
|
|
||
| # rubocop:disable Metrics/AbcSize |
There was a problem hiding this comment.
Hm. This actually exceeds the ABC size (which is already inflated) of any other method in the entire application. We should probably think about whether this method can be simplified, extracted, or factored differently. 🙂
There was a problem hiding this comment.
Thanks!
That's a tough one. I'm not very happy with my current solution, because we have to use corrector.replace(loc.operator, '') instead of corrector.remove(loc.operator)...
| if object.send_type? | ||
| return unless ASSIGNMENT_TYPES.include?(node.parent.parent.type) | ||
| end | ||
| def_node_matcher :array_new?, ARRAY_NEW_PATTERN |
There was a problem hiding this comment.
With this change, I would inline the pattern in #array_new? and call refer to that within #literal_expansion? instead. 🙂 (Might need to reorder the two pattern definitions.)
| @@ -80,12 +78,36 @@ def on_splat(node) | |||
|
|
|||
| private | |||
|
|
|||
| def unneeded_splat_expansion(node) | |||
| literal_expansion?(node) do |object| | |||
There was a problem hiding this comment.
Let's change "object" to something more meaningful. 🙂
akhramov
force-pushed
the
fix/unneeded-splat-expansion-array-new-in-literal
branch
from
78fe952 to
d52f7f4
Compare
| return false unless array_new?(array_new_node) | ||
|
|
||
| grandparent = array_new_node.parent.parent | ||
| grandparent.array_type? && grandparent.children.count > 1 |
There was a problem hiding this comment.
Use size instead of count. It is more efficient with no arguments.
|
|
||
| context 'when Array.new is expanded' do | ||
| context 'and the array contains more than one element' do | ||
| it 'does not autocorrect' do |
There was a problem hiding this comment.
We don't normally test that something isn't auto-corrected. This is redundant because autocorrect will not be called if there isn't an offense.
Currently `Lint/UnneededSplatExpansion` reports an offense when `Array.new` is used inside array literal: ```ruby foo = [1, 2, *Array.new(foo), 6] ``` Moreover, cop autocorrects this code to following: ```ruby foo = [1, 2, [Array.new(foo)], 6] ``` This change adds a check whether `Array.new` resides in an array literal, and if it does, the cop acts as following depending on amount of elements in the literal: | Example | Offense reported? | Autocorrects to | |------------------------|--------------------|------------------| | `[*Array.new(foo)]` | Yes | `Array.new(foo)` | | `[*Array.new(foo), 1]` | No | N/A |
akhramov
force-pushed
the
fix/unneeded-splat-expansion-array-new-in-literal
branch
from
d52f7f4 to
2c20986
Compare
Currently
Lint/UnneededSplatExpansionreports an offense whenArray.newis used inside array literal:Moreover, cop autocorrects this code to following:
This change adds a check whether
Array.newresides in an arrayliteral, and if it does, the cop acts as following depending on
amount of elements in the literal:
[*Array.new(foo)]Array.new(foo)[*Array.new(foo), 1]Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).rake spec) are passing.rake internal_investigation.and description in grammatically correct, complete sentences.
rake generate_cops_documentation(required only when you've added a new cop or changed the configuration/documentation of an existing cop).