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

Add Autocorrect for Lint/UselessAssignment cop #11597

Closed
edbarbera opened this issue Feb 19, 2023 · 6 comments
Closed

Add Autocorrect for Lint/UselessAssignment cop #11597

edbarbera opened this issue Feb 19, 2023 · 6 comments

Comments

@edbarbera
Copy link

Is your feature request related to a problem? Please describe.

The Lint/UselessAssignment cop often appears when linting tests. I would like to be able to run auto-correct to fix these linting errors.

Describe the solution you'd like

A method in the cop to auto-correct the useless assignment (happy to write it myself).

Example:

# Variable only assigned once, never referenced again
item = create(:item)

# Autocorrect to
create(:item)

Describe alternatives you've considered

I have searched around the repo to see if this has been mentioned in a prior Issue or PR, but couldn't find anything. I also don't see any reason to not allow an auto-correct method to automatically fix this error, but if there is please correct me.

Additional context

N/A

@vlad-pisanov
Copy link

In a lot of cases, you'd want the whole line gone, not just the assignment -- so you don't end up with things like:

item = 42

# autocorrect to 😬
42

@koic
Copy link
Member

koic commented Feb 20, 2023

Removing a whole line is unsafe change when RHS as a method call with side effects is used instead of 42. So it would not be the cop's responsibility to remove the whole line I think.

@vlad-pisanov
Copy link

vlad-pisanov commented Feb 20, 2023

@koic yeah, that's what I was getting at -- in some cases you'd want the assignment removed, in others you'd want the whole line removed -- which is something only the developer can decide correctly, I think

unused = 42                       # should be safe to remove line
unused = some_useful_call         # dev should decide (keep `some_useful_call`)
unused = some_useless_call        # dev should decide (remove whole line)

EDIT: actually, no -- automatically removing the whole line is unsafe, as it could be the result of a method/block: 😨

def foo
  unused = 42
end

@dduugg
Copy link
Contributor

dduugg commented Feb 22, 2023

In a lot of cases, you'd want the whole line gone, not just the assignment -- so you don't end up with things like:

item = 42

# autocorrect to 😬
42

…but this is handled by another cop (Lint/Void)

@edbarbera
Copy link
Author

edbarbera commented Feb 23, 2023

@vlad-pisanov definitely a good point that it would be unsafe to remove the entire line and that wasn't what I intended as I didn't think that the cop would need to handle a case such as (as @dduugg just mentioned):

unused = 42

In my head, the cop would only auto-correct when the RHS was a method call by using one of the inbuilt Rails methods? I haven't tested anything yet in code, so not sure how doable this approach is yet. However wanted to get people's opinions of whether or not this was a sensible auto-correction.

@r7kamura
Copy link
Contributor

r7kamura commented May 6, 2023

I have prepared the following pull requests for this issue. If you guys are interested, please take a look:

@bbatsov bbatsov closed this as completed in b0c64fd May 7, 2023
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

5 participants