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

Is EndAlignment not a Layout cop? #4704

Closed
bquorning opened this issue Sep 6, 2017 · 11 comments
Closed

Is EndAlignment not a Layout cop? #4704

bquorning opened this issue Sep 6, 2017 · 11 comments
Labels

Comments

@bquorning
Copy link
Contributor

In #4278, a bunch of whitespace cops were moved into the new “Layout department”. Since, I have wondered why Lint/EndAlignment wasn’t moved along with them.

@jonas054
Copy link
Collaborator

jonas054 commented Sep 6, 2017

Layout was added mostly to off-load Style, and it didn't occur to me that there were cops dealing with whitespace in other departments too. But it would make sense to move it. Especially since it makes rubocop --auto-correct --only Layout work better. We can still configure it to be on Warning level.

@bquorning
Copy link
Contributor Author

I think the Layout namespace makes a lot of sense. My .rubocop.yml files almost entirely consist of changing the enforced styles of Layout cops.

I didn’t really know what was the difference between Lint and Style, and I have never used the severity level feature of RuboCop. I just now read this part of the manual:

Each cop has a default severity level based on which department it belongs to. The level is warning for Lint and convention for all the others. Cops can customize their severity level. Allowed params are refactor, convention, warning, error and fatal.

I actually think warning is the wrong level for Lint/EndAlignment, and would propose to just move the cop into Layout and be on Convention level.

Probably Lint/BlockAlignment, Lint/ConditionPosition, and Lint/DefEndAlignment can be moved, too?

@jonas054
Copy link
Collaborator

jonas054 commented Sep 6, 2017

These cops are in the Lint department to indicate that the offenses are possibly bugs, and not just bad style choices. For that reason I want the level to be warning, but I support the idea of moving all of them.

@bquorning
Copy link
Contributor Author

I don’t immediately see how any of the offenses caught by these 4 cops might be a bug. Can you give any examples?

@Drenmi
Copy link
Collaborator

Drenmi commented Sep 6, 2017

I was looking through the style guide (and the internet) but can't find any examples of bugs caused by misaligned end without finding anything. Could be that it's from older Ruby versions, or that it was just put there arbitrarily?

I agree it would be convenient to have these together in Layout.

@mikegee
Copy link
Contributor

mikegee commented Sep 6, 2017

Edit: this code makes an offense from Layout/IndentationWidth not Lint/EndAlignment. (derp)

Perhaps it is a warning in Rubocop because it is a warning in Ruby:

% cat test/test.rb
begin
  foobar
  end
% ruby -cW2 test/test.rb
test/test.rb:3: warning: mismatched indentations at 'end' with 'begin' at 1
Syntax OK

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 6, 2017

I was looking through the style guide (and the internet) but can't find any examples of bugs caused by misaligned end without finding anything. Could be that it's from older Ruby versions, or that it was just put there arbitrarily?

MRI treats this as a warning and that's the reason why it's a lint cop in RuboCop as well. I think the reasoning was that you might not perceive properly which scope are you operating in because of the broken alignment.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 6, 2017

Perhaps it is a warning in Rubocop because it is a warning in Ruby:

Exactly.

@mikegee
Copy link
Contributor

mikegee commented Sep 6, 2017

Exactly.

I'm so sorry for the confusion, but the code I posted is not an offense for EndAlignment. This "bad" example from the Lint/EndAlignment docs raises no warnings from Ruby:

variable = if true
    end

I got mixed up by the names of these cops.

@bquorning
Copy link
Contributor Author

I see Ruby warnings for Lint/EndAlignment and Lint/DefEndAlignment offenses, but not for Lint/BlockAlignment and Lint/ConditionPosition:

» cat test.rb

# Lint/EndAlignment
class Test
  end

# Lint/DefEndAlignment
def test
  end

# Lint/BlockAlignment
test do
  end

# Lint/ConditionPosition
if
true
"true"
elsif
false
"false"
end
» ruby -w test.rb
test.rb:3: warning: mismatched indentations at 'end' with 'class' at 2
test.rb:7: warning: mismatched indentations at 'end' with 'def' at 6

@bquorning
Copy link
Contributor Author

Also relevant comment: #1789 (comment)

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.

bquorning added a commit to bquorning/rubocop that referenced this issue Jan 6, 2018
Oftentimes, a misaligned `end` on module/class definition is just a
whitespace/indentation error. Having it fixed together with the other `Layout`
cops by e.g. `rubocop --auto-correct --only Layout` is very convenient.

But since it *may* be something more serious than just a style issue, cop
violations are still of severity `warning`.
bbatsov pushed a commit that referenced this issue Jan 6, 2018
Oftentimes, a misaligned `end` on module/class definition is just a
whitespace/indentation error. Having it fixed together with the other `Layout`
cops by e.g. `rubocop --auto-correct --only Layout` is very convenient.

But since it *may* be something more serious than just a style issue, cop
violations are still of severity `warning`.
cheezenaan added a commit to cheezenaan-sandbox/sample_app_rev4 that referenced this issue Apr 23, 2018
* Style/TrailingCommaInLiteral is splited into two new cops since v0.53.0
  * see. rubocop/rubocop#5404
* Lint/EndAlignment is moved to Layout namespace
  * see. rubocop/rubocop#4704
cheezenaan added a commit to cheezenaan-sandbox/sample_app_rev4 that referenced this issue Apr 23, 2018
* Style/TrailingCommaInLiteral is splited into two new cops since v0.53.0
  * see. rubocop/rubocop#5404
* Lint/EndAlignment is moved to Layout namespace
  * see. rubocop/rubocop#4704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants