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

New Cop Style/MultilineMethodCallBraceLayout #2766

Merged

Conversation

panthomakos
Copy link
Contributor

Identical to Style/MultilineArrayBraceLayout,
Style/MultilineMethodDefinitionBraceLayout, and
Style/MultilineHashBraceLayout, except for method calls.

Includes a change to the MultilineLiteralBraceLayout mixin that allows
for specifying a different method for retrieving a node's children.

Identical to `Style/MultilineArrayBraceLayout`,
`Style/MultilineMethodDefinitionBraceLayout`, and
`Style/MultilineHashBraceLayout`, except for method calls.

Includes a change to the `MultilineLiteralBraceLayout` mixin that allows
for specifying a different method for retrieving a node's children.
@panthomakos panthomakos force-pushed the multiline-method-call-brace-layout branch from b310f46 to 4423146 Compare February 3, 2016 17:04
@@ -38,16 +38,20 @@ def autocorrect(node)

private

def children(node)
node.children
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the benefits of having this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a method call (on_send) the value of node.children is actually the receiver, method name and then arguments. For instance, parsing

something.foo(1,2,3)

returns (pseudo-code)

[something, :foo, 1, 2, 3]

This is not the case for all other types (hash, array, method definitions) which only return hash elements, array elements or method parameters. This method allows the same code to be re-used for method calls.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

What about:

method(a, b,
       c, d)

The braces are not symmetrical, but this is super common...

@panthomakos
Copy link
Contributor Author

@bbatsov That case is accounted for because the first argument is on the same line as the opening brace and the last argument is on the same line as the closing brace. That is valid under the cop's checks. It would be invalid to have either of the following:

method(a, b,
       c, d
)

method(
  a, b,
  c, d)

@panthomakos
Copy link
Contributor Author

Admittedly these rules make the most sense for hashes and arrays, but I think providing these cops for method definitions and calls makes sense because it provides a consistent way to deal with multi-line "things" surrounded by any kind of brace ([ for array, ( for methods, { for hash).

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

I agree. Just make it clear in the cop's description and in the examples that this is something considered good.

@panthomakos
Copy link
Contributor Author

I've used the same language for the array, hash and method definition ones. Do you have any suggestions on how to update the language?

original: "Checks that the closing brace in a method call is symmetrical with respect to the opening brace and the method arguments."

suggestion: "Checks that the closing brace in a method call is positioned in the same way as the opening brace with respect to the method arguments and line breaks."

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2016

I'm on the fence about the two texts. But I was a bit uncertain about the meaning of the wording prior to discussing it with you, so I'm guessing we can do better in terms of clarity. :-)

bbatsov added a commit that referenced this pull request Feb 4, 2016
…-layout

New Cop Style/MultilineMethodCallBraceLayout
@bbatsov bbatsov merged commit 2f6763b into rubocop:master Feb 4, 2016
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 this pull request may close these issues.

2 participants