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

Lint/EndAlignment disabled by default (was After Style/ConditionalAssignment auto-correct Lint/EndAlignment does not auto-correct) #2719

Closed
madwort opened this issue Jan 26, 2016 · 10 comments · Fixed by #2800

Comments

@madwort
Copy link
Contributor

madwort commented Jan 26, 2016

if var1 == 'string1'
  var2 = 'strong1'
elsif var1 == 'string2'
  var2 = 'strong2'
else
  var2 = 'strong3'
end

gets auto-corrected to:

var2 = if var1 == 'string1'
         'strong1'
       elsif var1 == 'string2'
         'strong2'
       else
         'strong3'
end

and Lint/EndAlignment refuses to auto-correct this.

Tested with rubocop version 0.36.0

@rrosenblum
Copy link
Contributor

Auto-correct Lint/EndAlignment is turned off by default. To enable auto-correct, add the following to your .rubocop.yml

Lint/EndAlignment:
  AutoCorrect: true

I am not sure why auto-correct is disabled on this cop by default. So far, it seems to work just fine to me. Maybe @bbatsov, @jonas054, or @lumeet could shed some light on this. If none of them are opposed, I am for enabling auto-correct on Lint/EndAlignment.

@alexdowad
Copy link
Contributor

If none of them are opposed, I am for enabling auto-correct on Lint/EndAlignment.

Same here.

@lumeet
Copy link
Contributor

lumeet commented Jan 27, 2016

Here's the original reasoning by @jonas054: #1789 (comment). I'm fine either way.

@stamhankar999
Copy link

+1 on enabling auto-correct

@madwort madwort changed the title After Style/ConditionalAssignment auto-correct Lint/EndAlignment does not auto-correct Lint/EndAlignment disabled by default (was After Style/ConditionalAssignment auto-correct Lint/EndAlignment does not auto-correct) Jan 30, 2016
@jonas054
Copy link
Collaborator

jonas054 commented Feb 5, 2016

If we enale auto-correct by default for EndAlignment and DefEndAlignment, then we're saying that the alignment of end keywords are a style issue and not a lint issue. I.e., it does not indicate a programming error, it's just ugly. In that case we also need to move those cops to Style/.

@rrosenblum
Copy link
Contributor

The end alignment cops are in the Lint category because the bad indentation could be something more serious than just a style issue. It could be a mistake in which keyword you think you're matching with the end. (ruby -w warns for these too, by the way.) So for this reason I don't think we can add auto-correct for these cops.

The original intention for leaving auto-correct disabled makes sense. The concept of EndAlignment almost takes on a double meaning because it could refer to a syntax error for incorrect keyword matching or a stylistic error because it is misaligned. I think in practice, this cop is used more for helping stylistically. I would be OK with moving it to a Style cop.

@alexdowad
Copy link
Contributor

I think there is a better solution; make ConditionalAssignment fix up indentation (including indentation of the end keyword) when it autocorrects.

In practice the conditional indentation cop is the only place where we are getting complaints about end indentation.

@rrosenblum
Copy link
Contributor

If I need to code ConditionalAssignment to handle alignment, then I will. Quickly looking at it again, correcting the end in the cop may be easier than I originally expected. It does raise the question of when should a cop be responsible for its own formatting and alignment, and when should it be left up to other cops to handle?

@alexdowad
Copy link
Contributor

when should a cop be responsible for its own formatting and alignment, and when should it be left up to other cops to handle?

Personally, I think the ideal situation is that cops never create offenses for other cops to correct. But that is probably not achievable. It doesn't matter much if some cops occasionally create new offenses.

@jonas054
Copy link
Collaborator

jonas054 commented Feb 6, 2016

@alexdowad I agree.
@rrosenblum Great if you can fix this in ConditionalAssignment.

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

Successfully merging a pull request may close this issue.

6 participants