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

Style/ParallelAssignment for array swapping #2464

Closed
ShockwaveNN opened this issue Nov 30, 2015 · 9 comments
Closed

Style/ParallelAssignment for array swapping #2464

ShockwaveNN opened this issue Nov 30, 2015 · 9 comments

Comments

@ShockwaveNN
Copy link
Contributor

I have code like this. I just need to swap two elements in array

array = [1, 2, 3, 4]
array[0], array[1] = array[1], array[0]
p array

rubocop game me this offence:

test.rb:2:1: C: Style/ParallelAssignment: Do not use parallel assignment.
array[0], array[1] = array[1], array[0]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

By documentation of Style/ParallelAssignment cop I cannot understand that is wrong with my code.
Is this a bug in cop, or there is better way to swap elements in my array?

I use
rubocop 0.35.1 (using Parser 2.2.3.0, running on ruby 2.1.6 x86_64-linux)

@alexdowad
Copy link
Contributor

Is this a bug in cop

Yes. It's a bug.

This cop tries to find an ordering of assignments which doesn't change the meaning of the code. If it can't find one, it's not supposed to issue a warning.

@kamen-hursev
Copy link

Just a clarification, Rubocop has the same bug for swapping values of hash or object attributes:
All these result in Style/ParallelAssignment offences:

array[0], array[1] = array[1], array[0]
hash[:a], hash[:b] = hash[:b], hash[:a]
point.x, point.y = point.y, point.x

@rrosenblum
Copy link
Contributor

I think the issue stems from the cop seeing these as method calls instead of as assignments. I will try to dig into this cop to fix it.

@rrosenblum
Copy link
Contributor

@alexdowad I may need some help with this one. There was a change in the code to start using NodePattern to check find_valid_order. I am not too familiar with NodePattern. I am sure I could figure it out, but you can probably fix the bug faster than I can. My first thought was to reintroduce the swapping_variables? which was replaced by find_valid_order.

@alexdowad
Copy link
Contributor

@rrosenblum, no problem. I am just going for dinner now, but will look at it later.

@alexdowad
Copy link
Contributor

My first thought was to reintroduce the swapping_variables? which was replaced by find_valid_order

swapping_variables? was replaced for good reason. It was only able to identify very basic cases where parallel assignment cannot validly be replaced. find_valid_order is far more robust. It just needs to be updated to handle aref assignments and attribute assignments.

@rrosenblum
Copy link
Contributor

find_valid_order is far more robust. It just needs to be updated to handle aref assignments and attribute assignments.

After looking at the cop, that is what I was thinking. I had some guesses with how it would need to be modified, but I would need to play with it to really figure out how it needs to be changed.

@alexdowad
Copy link
Contributor

Hey @rrosenblum... just opened a PR which fixes this one.

If you are interested, try reading the comments at the top of node_pattern.rb. The intention is that other developers should be able to pick up the "node pattern" language within 10 minutes -- but if there's anything which is hard to grasp, I will try to make the docs clearer.

@rrosenblum
Copy link
Contributor

I just read over the docs at the top of node_pattern.rb, they seem detailed enough to me. The idea behind NodePattern makes sense to me. I will need to play around with it before I am completely comfortable with it.

alexdowad added a commit to alexdowad/rubocop that referenced this issue Dec 10, 2015
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

4 participants