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

Indentation of if when using the return value #661

Closed
moonglum opened this issue Dec 6, 2013 · 8 comments · Fixed by #665
Closed

Indentation of if when using the return value #661

moonglum opened this issue Dec 6, 2013 · 8 comments · Fixed by #665
Assignees

Comments

@moonglum
Copy link

moonglum commented Dec 6, 2013

Currently, the following code is not ok in rubocop's opinion:

x = if true
  2
else
  3
end

What Rubocop wants is the following:

x = if true
      2
    else
      3
    end

Can I configure Rubocop to take the first version instead? Because I really prefer that!

Thanks for Rubocop 👍 It helps a lot 😸

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 6, 2013

Currently not. I've said to @jonas054 last evening we should probably make this configurable, in light of a related change for case. Personally, I'm extremely fond of (which is fine by RuboCop):

x = 
  if true
    2
  else
    3
  end

Anyways, @jonas054 will you look into this?

@ghost ghost assigned jonas054 Dec 6, 2013
@jonas054
Copy link
Collaborator

jonas054 commented Dec 6, 2013

Yes.

@moonglum
Copy link
Author

moonglum commented Dec 8, 2013

Thanks 😸

@jonas054
Copy link
Collaborator

jonas054 commented Dec 8, 2013

No probelm. Well... some. There are two cops that report indentation related offences for the example code:

/tmp/exaple_661.rb:2:3: C: IndentationWidth: Use 2 (not -2) spaces for indentation.
  2
  ^
/tmp/exaple_661.rb:4:3: C: IndentationWidth: Use 2 (not -2) spaces for indentation.
  3
  ^
/tmp/exaple_661.rb:5:1: W: EndAlignment: end at 5, 0 is not aligned with if at 1, 4
end
^^^

The double reporting is not a big problem as long as we consider the inspected code to be incorrect, but if we want to allow it through configuration settings, then having to change two configuration parameters in two different cops becomes a problem IMO.

The way I want to solve this is to introduce a configuration parameter in the EndAlignment cop, which is a lint cop, and then just make IndentationWidth accept both styles of indentation without configuration.

@moonglum
Copy link
Author

moonglum commented Dec 8, 2013

From a user perspective: Sounds good to me 😄

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 8, 2013

@jonas054 This sounds reasonable, but there's something more to consider - the else alignment in constructs like if/else, unless/else. Maybe there should be a separate cop for them (like the case cop).

@jonas054
Copy link
Collaborator

jonas054 commented Dec 8, 2013

Separate cop sounds like a good idea. ElseIndentation should check that else is indented as deep as end.

@moonglum
Copy link
Author

Thank you so much 👍

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.

3 participants