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

Add Style/MultilineAssignmentLayout Cop #2361

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

panthomakos
Copy link
Contributor

This cop enforces the width efficient style of multi-line assignments.
It guarantees that an assignment in which the right-hand-side spans
multiple lines includes a newline after the assignment operator.

  • By default only if and case nodes are supported.
  • Indentation can further by corrected by other cops such as
    Style/IndentationConsistency.

https://github.com/bbatsov/ruby-style-guide#indent-conditional-assignment

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from 9a24ea4 to f01daea Compare October 28, 2015 18:17
end

it 'auto-corrects offenses' do
new_source = autocorrect_source(cop, "blarg = if true\nend")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if you used an array of lines instead of a single string with newlines, as we do in most specs.

@jonas054
Copy link
Collaborator

This cop is useful only if Lint/EndAlignment: AlignWith is variable. It might be better to check that configuration in the cop and not do anything if the configuration is keyword. Then the cop could be enabled by default.

@panthomakos
Copy link
Contributor Author

@jonas054 It seems to me that enabling this cop only when Lint/EndAlignment is set to a particular value could result in some problems. For example, if this cop is configured w/ more SupportedTypes (for example if it excludes if and case but includes kwbegin or array) then the behavior is confusing.

The reason I am providing if and case as the default SupportedTypes is because they are mentioned in the Style Guide explicitly.

If you think this causes too much confusion I could also enable more types by default or just allow all types by default and allow users to pair the list down on their own.

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from f01daea to feb16ec Compare October 29, 2015 17:21
@panthomakos
Copy link
Contributor Author

@jonas054 I adjusted the specs to use an array of lines as well.

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from feb16ec to 1753e43 Compare October 30, 2015 04:47
@jonas054
Copy link
Collaborator

Having looked a bit closer now, I see that this cop checks more than is covered by the linked rule in the style guide.

Since we're linking to the rule in the configuration, I think the best way is to hard-code the list of supported types. They should be if (this node type also includes unless), while, until, case, for (although it's not useful to assign the return value), and maybe kwbegineven though it's not a conditional. Any other? I'm not sure. But not array.

And since the style guide says

# good - it's apparent what's going on
kind = case year
       when 1850..1889 then 'Blues'
       when 1890..1909 then 'Ragtime'
       when 1910..1929 then 'New Orleans Jazz'
       when 1930..1939 then 'Swing'
       when 1940..1950 then 'Bebop'
       else 'Jazz'
       end

We must allow code like that in the cop. Looking at the configuration of Lint/EndAlignment is perhaps not the best solution - that cop might be disabled.

@panthomakos
Copy link
Contributor Author

@jonas054

I'm not objected to hard-coding the values at all. It sounds like I should use %w(if while until case kwbegin). Should class, module and begin be included?

Do you think it's best to leave the cop disabled by default; and just allow users to enable if they want to use this style?

@jonas054
Copy link
Collaborator

Nodes of type begin are wrappers for sequences of nodes. They're not bound to a specific keyword, and therefore don't belong in our list. The class and module nodes, on the other hand, while not being conditionals, should be included since they're similar enough to what the rule describes (IMO). Just like kwbegin.

I believe that we can enable the cop by default, if we limit it a bit as per our discussion above.

@panthomakos
Copy link
Contributor Author

@jonas054 The list sounds good. I'll get working on that. I am still a bit confused about when to enable/disable the cop though and if it belongs in enabled.yml or disabled.yml. Can you clarify that part?

@jonas054
Copy link
Collaborator

Put it in enabled.yml. If RuboCop's own source code passes its inspection, the cop can be enabled by default, I think.

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch 2 times, most recently from b7c5525 to 5d07c13 Compare November 2, 2015 18:22
@panthomakos
Copy link
Contributor Author

@jonas054 I've updated w/ the pre-set list of SUPPORTED_STYLES that appear to make sense to me. The rubocop source does not pass the cop by default, so I've left it in disabled.yml. For example:

https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/config_store.rb#L31

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 2, 2015

Perhaps it should have configurable styles. My main concern about this cop is its interplay with the end alignment cop, as you've already discussed a bit. I see the point of the cop, but I'm worried we'll make a complicated setup even more complex.

@panthomakos
Copy link
Contributor Author

I am open to any suggestions to make this cop fit into the existing RuboCop ecosystem. It seems that having a pre-set list of "things that return" - excluding arrays and hashes - is a good starting point. That at least covers most use cases.

The list could still be configurable to allow for the removal/addition of some types for advanced usage.

It does seem reasonable to me to not enable/disable functionality based on another cop's configurations. My perception of this is that it would lead to unnecessary complexity and additional trouble in debugging issues. One beauty of the RuboCop system is how modularized each cop can be.

Of course, I am not as familiar with the overall architecture of RuboCop or it's uses, so I'm happy to implement this in the way you think works best.

@jonas054
Copy link
Collaborator

jonas054 commented Nov 3, 2015

I was perhaps wrong in suggesting hard-coding the list. If you want to bring back the config option, it's fine with me.

If we don't want to introduce a dependency towards the configuration of Lint/EndAlignment, then having this cop disabled by default is a good choice.

There is one problem. If you run the cop with --auto-correct, it produces ugly code because we don't have a cop that fixes the indentation of the line following =. But I guess that's not a problem for this cop. We need to add another one.

@panthomakos
Copy link
Contributor Author

@jonas054 I'll add the config option back.

It sounds like maybe a IndentAssignment cop would solve this problem? It would register/autocorrect the alignment of the first element of a multi-line assignment when the first element and the assignment are on different lines. Much like: https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/style/indent_array.rb

Should I add that to this PR or create a separate PR?

@panthomakos
Copy link
Contributor Author

@jonas054 Just a little more guidance regarding the IndentAssignment cop and if I should add it to this PR. Once I get some guidance there I can finish this up.

@jonas054
Copy link
Collaborator

jonas054 commented Nov 7, 2015

@panthomakos Sorry for the late reply. Add the new cop in a separate PR.

An IndentAssignment cop sounds good. There might be other examples of expressions where we don't check the indentation after a line break, but assignment is the one we're aware of now, so let's solve that problem first.

@panthomakos
Copy link
Contributor Author

@jonas054 I added #2410. It seems reasonable to me to wait for that PR to be merged first, that way the autocorrect code will be properly formatted. I am going to address the additional comments on this PR once #2410 is resolved.

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from 5d07c13 to e105f8f Compare November 30, 2015 23:18
@panthomakos
Copy link
Contributor Author

@jonas054 @bbatsov PTAL I've updated this PR now that Style/IndentAssignment is available.

# foo =
# begin
# compute
# resucue => e

Choose a reason for hiding this comment

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

Nit: spelling rescue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@jonas054
Copy link
Collaborator

jonas054 commented Dec 1, 2015

👍 Looks good to me.

@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from e105f8f to 214496d Compare December 1, 2015 19:05
@panthomakos
Copy link
Contributor Author

Fixed spelling of rescue and rebased off master.

include CheckAssignment

MSG = 'Right hand side of multi-line assignment is on the same ' \
'line as the assignment.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

"assignment operator = " will be clearer.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 1, 2015

Apart from my small comments, the code looks good. My main concern is that this is not configurable - I can imagine some people would want to make sure the thing on the right side is on the same line as =. People who are fond of styles like:

a = if ala
  tralala
end

@panthomakos
Copy link
Contributor Author

@bbatsov I'll take a look at making this configurable and I'll incorporate your other comments.

This cop checks for a new-line after the assignment operator in
multi-line assignments.

When `EnforcedStyle` is set to `new_line`, this cop enforces the width
efficient style of multi-line assignments. It guarantees that an
assignment in which the right-hand-side spans multiple lines includes
a newline after the assignment operator.

When `EnforcedStyle` is set to `same_line`, this cop guarantees that
an assignment in which the right-hand-side spans multiple lines begins
on the same line as the assignment operator.

* A configurable set of node types, like `if` and `case` are supported.
* Indentation can further by corrected by other cops such as
`Style/IndentationConsistency` and `Style/IndentAssignment`.

https://github.com/bbatsov/ruby-style-guide#indent-conditional-assignment
@panthomakos panthomakos force-pushed the multiline-assignment-layout branch from 214496d to 353c1eb Compare December 1, 2015 21:50
@panthomakos
Copy link
Contributor Author

@bbatsov @jonas054 PTAL

I've added an EnforcedStyle which supports same_line or new_line. Unfortunately I could not turn this on by default because the RuboCop source code is not consistent, so I've left the cop in disabled.yml.

@jonas054
Copy link
Collaborator

jonas054 commented Dec 9, 2015

👍 Looks good to me.

bbatsov added a commit that referenced this pull request Dec 9, 2015
@bbatsov bbatsov merged commit 308c456 into rubocop:master Dec 9, 2015
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.

4 participants