Add Style/LineBreak Cops #2277

panthomakos · 2015-09-25T23:24:47Z

When large multi-line lists are present in a code base it's often
rather arbitrary to align them based on the first line/element:

long_variable = { foo: 'foo',
                  bar: 'bar',
                  baz: 'baz' }

In this example if the variable name is refactored the entire list
formatting also needs to change. This makes diffs, in particular, very
hard to read due to the large number of formatting-only changes:

short = { foo: 'foo',
          bar: 'bar',
          baz: 'baz' }

These alignments also greatly reduce the available space on a line for
each element, especially if there is a maximum line length enforcement.

The Style/MultilineList cop along with any appropriate Align or Indent
cop (for example HashAlign and HashIndent) allows you to enforce a
more diff-friendly and short-line style:

variable = {
  foo: 'foo',
  bar: 'bar',
  baz: 'baz' }

Mutually-exclusive configuration options are provided for each of method
parameters, method arguments, array elements and hash elements. Care has
been taken to ensure that DSL-like method calls are ignored and do not
result in invalid Ruby.

I have added tests for all use-cases I could find in some large Ruby and
Rails code-bases.

jonas054 · 2015-09-26T17:23:11Z

lib/rubocop/cop/style/multiline_list.rb

+      class MultilineList < Cop
+        MSG = 'Move all elements below the first line of a multi-line list.'
+
+        def on_def(node)


You need to handle defs nodes too, and you can do this by using the OnMethodDef mixin.

panthomakos · 2015-09-27T04:12:05Z

jonas054 Thank you very much for the review. I've addressed all of your comments and amended my commit. The autocorrect method is also a lot simpler because I am using the first element as the offense (instead of the parent node) which means inserting a newline is much simpler. PTAL

jonas054 · 2015-09-27T05:19:34Z

config/default.yml

@@ -525,6 +525,13 @@ Style/MethodName:
    - snake_case
    - camelCase

+Style/MultilineList:


The cop is enabled by default but each part is disabled by default. I think it would be better to do it the other way around, because then people can find it when they look in disabled.yml for extra things to turn on.

jonas054 · 2015-09-27T06:11:49Z

That's much better. I've added some more comments, mostly because I didn't get around to do it earlier.

panthomakos · 2015-09-28T15:49:13Z

@jonas054 I've addressed your comments. Thanks again for the review. I'm learning a lot about Ruby code parsing :)

PTAL

panthomakos · 2015-09-28T20:46:20Z

@jonas054 I am taking a look at some issues w/ the array assignment part. After making your changes I am getting some infinite loops for some implicit arrays in setters. I am investigating to see if I can write a simple test case and resolve the issue.

panthomakos · 2015-09-28T21:18:56Z

lib/rubocop/cop/style/multiline_list.rb

+
+        def assignment_on_same_line?(node)
+          source = node.loc.expression.source_line[0...node.loc.column]
+          source =~ /\s*\=\s*$/


@jonas054 I was unable to convert this using the lhs, rhs code you suggested because it does not work for some "assignment" types. It does work for masgn but not for a send type. For example:

config.values = :a, :b, :c

I see. Well, both assignment_on_same_line? and method_uses_parens? handle more than one node type, and then you have to do it with regexp matching or add handling of the different cases. I guess it's OK as it is.

panthomakos · 2015-09-28T21:19:26Z

@jonas054 I resolved the issue w/ implicit arrays and added a test case to demonstrate the issue. PTAL

panthomakos · 2015-09-28T21:55:23Z

lib/rubocop/cop/style/multiline_list.rb

+          return if children.size < 2
+
+          line = start.loc.line
+          min = children.min_by { |n| n.loc.line }


@jonas054 I have a question related to this code. Is there a standard way you check for "does this node span multiple lines"? For example, the following test case won't pass this implementation:

something('foo', bar: 1, baz: 2)

The reason is that even though there are two children, they both start on the same line. While I could make some kind of check to see if any of the individual children have a "\n" in their source, it seems like there might be a better way to address this issue. Do you know if there is some way I can go about check this so that even these cases are tagged as offenses?

Assuming that the node has a begin and an end, which I think it will in this case, you can use node.loc.begin.line and node.loc.end.line. The same will hold true for other attributes off of loc.

panthomakos · 2015-09-29T17:45:29Z

@rrosenblum I've incorporated your comments. I resolved the issue with elements spanning multiple lines by using the first_line and last_line of the loc object instead of just the line number. I've added a test case that passes with the updated code as well.

jonas054 · 2015-09-30T13:19:34Z

lib/rubocop/cop/style/multiline_list.rb

+            check_children(node, node.children)
+          elsif method_uses_parens?(node.parent, node)
+            check_children(node, node.children, node.parent)
+          else


else return isn't really needed.

panthomakos · 2015-09-30T15:56:47Z

Incorporated comments. Removed the else return.

jonas054 · 2015-10-01T07:12:50Z

👍 Looks good!

panthomakos · 2015-10-12T22:21:38Z

ping @jonas054 @rrosenblum I rebased off master again. Is there anything else I need to do here to get this change merged to master?

rrosenblum · 2015-10-13T13:01:23Z

@panthomakos the build is failing for an incorrect format in the CHANGELOG. There is a missing colon after the link to the PR.

Should be:

* [#2277](https://github.com/bbatsov/rubocop/pull/2277):

panthomakos · 2015-10-13T13:12:30Z

@rrosenblum fixed the CHANGELOG formatting

rrosenblum · 2015-10-13T13:12:37Z

spec/rubocop/cop/style/multiline_list_spec.rb

+      it 'autocorrects the offense' do
+        new_source = autocorrect_source(cop, source)
+
+        expect(new_source).to eq(


This doesn't seem like the correct auto-correction to me. The example given in the documentation comment shows

# good with EnableMethodParams def method( foo, bar, baz) do_something end

The auto-correct places the first element left justified with the def.

def foo( bar, baz) do_something end

Should it be indented or is it relying on another cop to handle that indention?

It relies on Style/FirstParameterIndetation:

http://www.rubydoc.info/gems/rubocop/0.29.1/RuboCop/Cop/Style/FirstParameterIndentation

Any additional alignment should then be fixed by Style/AlignParameters:

http://www.rubydoc.info/gems/rubocop/0.29.1/RuboCop/Cop/Style/AlignParameters

bbatsov · 2015-10-13T13:30:51Z

I haven't had time to review the code, but I can tell you that I definitely don't like the name of the cop, as it's not clear what it refers to. Certainly, the term "list" is not used often in Ruby.

panthomakos · 2015-10-13T13:42:01Z

@bbatsov I suppose the other option here is to have 4 separate cops that indicate that a line break is necessary, something like: FirstHashElementLinePlacement or FirstArrayElementLineBreak. Maybe I can generalize it to FirstMultilineElementLinePlacement?

bbatsov · 2015-10-15T20:12:25Z

Probably this should be a few separate cops. @jonas054 Thoughts?

jonas054 · 2015-10-15T20:28:59Z

I hadn't thought about that while reviewing, but sure, why not? It's more simple in a way to have four separate cops than one cop with four enable properties. And the naming should be more natural.

panthomakos · 2015-10-15T21:20:31Z

@bbatsov @jonas054

Please take another look. I've split the cop up into 4 separate cops with cleaner descriptions and messages.

jonas054 · 2015-10-16T05:01:24Z

👍 Looks good to me.

bbatsov · 2015-10-16T05:45:03Z

Looks good. Just remove the . in the commit message's title.

Btw, there should probably a set of complementary checks for the final parenthesis, so you can enforce either this style:

variable = {
  foo: 'foo',
  bar: 'bar',
  baz: 'baz' }

or this one (which is my preference):

variable = {
  foo: 'foo',
  bar: 'bar',
  baz: 'baz' 
}

* `Style/FirstArrayElementLineBreak` checks for a line break before the first element in a multi-line array. * `Style/FirstHashElementLineBreak` checks for a line break before the first element in multi-line hash. * `Style/FirstMethodArgumentLineBreak` checks for a line break before the first argument in a multi-line method call. * `Style/FirstMethodParameterLineBreak` checks for a line break before the first parameter in a multi-line method parameter definition. When large multi-line lists are present in a code base it's often rather arbitrary to align them based on the first element: ``` long_variable = { foo: 'foo', bar: 'bar', baz: 'baz' } ``` In this example if the variable name is refactored the entire list formatting also needs to change. This makes diffs, in particular, very hard to read due to the large number of formatting-only changes: ``` short = { foo: 'foo', bar: 'bar', baz: 'baz' } ``` These alignments also greatly reduce the available space on a line for each element, especially if there is a maximum line length enforcement. This collection of cops along with any appropriate Align or Indent cop (for example `HashAlign` and `HashIndent`) allows you to enforce a more diff-friendly and short-line style: ``` variable = { foo: 'foo', bar: 'bar', baz: 'baz' } ``` Care has been taken to ensure that DSL-like method calls are ignored and do not result in invalid Ruby. I have also added tests for all use-cases I could find in some large Ruby and Rails code-bases.

panthomakos · 2015-10-16T13:13:09Z

@bbatsov Removed period from commit message.

I prefer the second as well. I was thinking about adding that set of cops after this PR was complete.

Add Style/LineBreak Cops

bbatsov · 2015-10-16T15:57:54Z

I was thinking about adding that set of cops after this PR was complete.

Looking forward to those. :-)

This cop is inspired by a comment in the `Style/LineBreaks` cop PR: rubocop#2277 (comment) This cop checks that the closing brace in an array literal is symmetrical with respect to the opening brace and the array elements. I am very open to suggestions on naming and ways to clarify the offense messages. # bad [ :a, :b ] # bad [ :a, :b ] # good [ :a, :b ] #good [ :a, :b ] My intention is to expand this to account for hashes, method parameters and method arguments once the language and approach is validated.

jonas054 reviewed Sep 26, 2015
View reviewed changes

panthomakos force-pushed the style-multiline-list branch from 6f099cb to 6314d47 Compare September 27, 2015 04:10

jonas054 reviewed Sep 27, 2015
View reviewed changes

panthomakos force-pushed the style-multiline-list branch from 6314d47 to 762e5f4 Compare September 28, 2015 15:48

panthomakos force-pushed the style-multiline-list branch from 762e5f4 to 778707d Compare September 28, 2015 21:17

panthomakos reviewed Sep 28, 2015
View reviewed changes

panthomakos force-pushed the style-multiline-list branch from 778707d to 65d66c9 Compare September 29, 2015 17:44

jonas054 reviewed Sep 30, 2015
View reviewed changes

panthomakos force-pushed the style-multiline-list branch from 65d66c9 to 84a4788 Compare September 30, 2015 15:56

panthomakos force-pushed the style-multiline-list branch from 3750050 to 715d285 Compare October 13, 2015 13:11

rrosenblum reviewed Oct 13, 2015
View reviewed changes

panthomakos force-pushed the style-multiline-list branch from 715d285 to 8b68270 Compare October 15, 2015 21:19

panthomakos changed the title ~~Add Style/MultilineList Cop.~~ Add Style/LineBreak Cops. Oct 15, 2015

panthomakos force-pushed the style-multiline-list branch from 8b68270 to 6974e6f Compare October 15, 2015 21:19

panthomakos force-pushed the style-multiline-list branch from 6974e6f to e672339 Compare October 16, 2015 13:10

panthomakos changed the title ~~Add Style/LineBreak Cops.~~ Add Style/LineBreak Cops Oct 16, 2015

bbatsov added a commit that referenced this pull request Oct 16, 2015

Merge pull request #2277 from panthomakos/style-multiline-list

5c9fa95

Add Style/LineBreak Cops

bbatsov merged commit 5c9fa95 into rubocop:master Oct 16, 2015

panthomakos mentioned this pull request Dec 2, 2015

New Cop Style/ClosingArrayBraceLayout #2472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Style/LineBreak Cops #2277

Add Style/LineBreak Cops #2277

panthomakos commented Sep 25, 2015

jonas054 Sep 26, 2015

panthomakos commented Sep 27, 2015

jonas054 Sep 27, 2015

jonas054 commented Sep 27, 2015

panthomakos commented Sep 28, 2015

panthomakos commented Sep 28, 2015

panthomakos Sep 28, 2015

jonas054 Sep 30, 2015

panthomakos commented Sep 28, 2015

panthomakos Sep 28, 2015

rrosenblum Sep 29, 2015

panthomakos commented Sep 29, 2015

jonas054 Sep 30, 2015

panthomakos commented Sep 30, 2015

jonas054 commented Oct 1, 2015

panthomakos commented Oct 12, 2015

rrosenblum commented Oct 13, 2015

panthomakos commented Oct 13, 2015

rrosenblum Oct 13, 2015

panthomakos Oct 13, 2015

panthomakos Oct 13, 2015

bbatsov commented Oct 13, 2015

panthomakos commented Oct 13, 2015

bbatsov commented Oct 15, 2015

jonas054 commented Oct 15, 2015

panthomakos commented Oct 15, 2015

jonas054 commented Oct 16, 2015

bbatsov commented Oct 16, 2015

panthomakos commented Oct 16, 2015

bbatsov commented Oct 16, 2015

Add Style/LineBreak Cops #2277

Add Style/LineBreak Cops #2277

Conversation

panthomakos commented Sep 25, 2015

Choose a reason for hiding this comment

panthomakos commented Sep 27, 2015

Choose a reason for hiding this comment

jonas054 commented Sep 27, 2015

panthomakos commented Sep 28, 2015

panthomakos commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panthomakos commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panthomakos commented Sep 29, 2015

Choose a reason for hiding this comment

panthomakos commented Sep 30, 2015

jonas054 commented Oct 1, 2015

panthomakos commented Oct 12, 2015

rrosenblum commented Oct 13, 2015

panthomakos commented Oct 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Oct 13, 2015

panthomakos commented Oct 13, 2015

bbatsov commented Oct 15, 2015

jonas054 commented Oct 15, 2015

panthomakos commented Oct 15, 2015

jonas054 commented Oct 16, 2015

bbatsov commented Oct 16, 2015

panthomakos commented Oct 16, 2015

bbatsov commented Oct 16, 2015