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

Wrong correction in Style/IfUnlessModifier with first-line comment and code after the end block on the same line #8499

Closed
dmytro-savochkin opened this issue Aug 9, 2020 · 3 comments · Fixed by #8523
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted

Comments

@dmytro-savochkin
Copy link

I think Style/IfUnlessModifier doesn't correctly handle the case when there is a first-line comment (right after the if keyword) AND some code right after the end keyword. If you correct this code with rubocop -a it would put the comment right after the end keyword, before the code that was there previously. This basically makes the code invalid after correction.

This probably can be better seen in the example below.

I think in such a case we should stop auto-correction because it's pretty non-deterministic: where would that comment should go? Did it belong to the if-end condition? In this case it should go right after it, before the next code. But that's impossible in a single-line modifier form, unless we use these /* comments */ but we probably wouldn't want to do this, so better not to auto-correct.

I can provide a PR if it seems right.


Steps to reproduce the problem

We have file test.rb with the following code:

[
  1, if foo # this is due to ...
       2
     end, 3
]

We run rubocop test.rb --only Style/IfUnlessModifier -a.

Actual behavior

[
  1, (2 if foo) # this is due to ..., 3
]

Expected behavior

Everything stays the same and no offense is given (I think it's the right way):

[
  1, if foo # this is due to ...
       2
     end, 3
]

or somehow it's fixed to

[
  1, (2 if foo), 3 # this is due to ...
]

but in this case the comment is in the wrong place, or it's fixed to

[
  1, (2 if foo) /* # this is due to ... */, 3 
]

but we probably don't want to use this syntax for comments.

RuboCop version

0.89.0 (using Parser 2.7.1.4, rubocop-ast 0.2.0, running on ruby 2.5.1 x86_64-darwin18)

@marcandre
Copy link
Contributor

Linking to #8281; I think that WhileUntilModifier takes care of this but not IfUnlessModifier

They really need to be refactored...

@marcandre marcandre added bug good first issue Easy task, suitable for newcomers to the project help wanted labels Aug 10, 2020
@dmytro-savochkin
Copy link
Author

@marcandre , yeah, I agree but that they need to be updated, but what the expected behavior should be in the case described in this issue?

@marcandre
Copy link
Contributor

For sure, produce equivalent code and not loose comments.

Probably any comment (but after the last element) prevents an offense.

dmytro-savochkin pushed a commit to dmytro-savochkin/rubocop that referenced this issue Nov 6, 2020
… first-line comment and code after end block
bbatsov pushed a commit that referenced this issue Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Easy task, suitable for newcomers to the project help wanted
Projects
None yet
2 participants