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

Style/ClosingParenthesisIndentation Changes #4136

Closed
jfelchner opened this issue Mar 17, 2017 · 3 comments
Closed

Style/ClosingParenthesisIndentation Changes #4136

jfelchner opened this issue Mar 17, 2017 · 3 comments

Comments

@jfelchner
Copy link
Contributor

jfelchner commented Mar 17, 2017

I want to outline my goals here and try to get some discussion started.

Goal

I'd like to update the Style/ClosingParenthesisIndentation cop to be more robust and handle specific indentation scenarios. Currently the Style/ClosingParenthesisIndentation cop is pretty basic which unfortunately means that it doesn't achieve the desired result in a lot of cases.

Examples

        hello_this_is_my_variable = what.could.this_method_be_doing(
                                      i_dunno: 'but',
                                      i_like:  'it',
)

It's fairly clear from this code that the developer would want to align the parenthesis with the opening one. Because this:

        hello_this_is_my_variable = what.could.this_method_be_doing(
                                      i_dunno: 'but',
                                      i_like:  'it',
        )

looks ridiculous. However in the same codebase, there may be a case like this:

        what.could.this_method_be_doing(
          i_dunno: 'but',
          i_like:  'it',
)

where it's fairly obvious to the reader that the right paren should be aligned by the beginning of the line with the method call.

There's also an (albeit weirder) situation like this:

        what.could.this_method_be_doing(i_dunno: 'but',
                                        i_like:  'it',
)

Where the right paren should be aligned with the right paren. Even though that looks weird, it looks better than the current result of this cop, which is:

        what.could.this_method_be_doing(i_dunno: 'but',
                                        i_like:  'it',
        )

Possibilities

What I'd like to do is have a discussion about the possible changes to the Style/ClosingParenthesisIndentation which could cover these use cases.

@jfelchner
Copy link
Contributor Author

Ping @jonas054 @Drenmi 😁

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2017

I might be missing something, but it's unclear to me whether you're reporting a bug or proposing an improvement.

@jfelchner
Copy link
Contributor Author

@bbatsov improvement for the most part. I believe the cop only handles one simple case. I'm asking for feedback on configuration changes that would allow it to handle the cases outlined above.

bbatsov pushed a commit that referenced this issue Apr 23, 2018
I think the best approach for this cop is to remove most of the logic
and let it depend on where the parameters are aligned at.  I think there
should be only two rules:

If the method is called (or defined) and the arguments/parameters...

* that span more than one line, the closing parenthesis should be
  outdented by one `IndentationWidth` from the last argument/parameter
* that are all on one line, the closing parenthesis should be aligned
  with the opening parenthesis

This lets the other cops like `Layout/FirstParameterIndentation`,
`Layout/FirstMethodParameterLineBreak`,
`Layout/FirstMethodArgumentLineBreak`,
`Layout/MultilineMethodDefinitionBraceLayout`,
`Layout/MultilineMethodCallBraceLayout` and `Layout/AlignParameters` be
responsible for their jobs and `Layout/ClosingParenthesisIndentation`
would simply be the result of the end of that flow.

By simplifying the cop, it allows for all of the cases described in #5650,
all of my use cases here, and will keep almost every current
`ClosingParenthesisIndentation` spec passing.

References #4136, #5650, #3204, #3139, #2791
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

2 participants