New braces_for_chaining style for Style/BlockDelimiters. #2329

panthomakos · 2015-10-20T04:48:00Z

As per the style-guide, even though multi-line chaining
is considered ugly and we should generally find ways to re-write the
code, sometimes this simply isn't possible. Regardless, some code-bases
would like to allow multi-line blocks to be used in method chains.

We can also argue that multi-line chaining, when used, is better like
this:

array.map { |x|
  x * 2
}.map(&:to_s)

than like this:

array.map do |x|
  x * 2
end.map(&:to_s)

This style, when selected, forces multi-line blocks to use braces if
their return value is being chained with another method. I believe
that this is a better option than the current default configuration
which enforces the second of the two code examples.

jonas054 · 2015-10-22T04:41:28Z

This is already taken care of if you configure

Style/BlockDelimiters:
  EnforcedStyle: semantic

panthomakos · 2015-10-22T18:21:00Z

@jonas054 This is a little bit different because semantic requires {} on assignment as well:

Style/BlockDelimiters:
  EnforcedStyle: semantic

# good
variable = array.map { |i|
  i*2
}

This PR allows you to enforce do-end for multi-line blocks except when chaining methods:

Style/BlockDelimiters:
  EnforcedStyle: line_count_based
  EnforceMultiLineBraceChaining: true

# good
variable = array.map do |i|
  i * 2
end

# good
array.map{ |i|
  i * 2
}.map(&:to_sym)

jonas054 · 2015-10-24T04:12:17Z

CHANGELOG.md

@@ -17,6 +17,7 @@
 * [#2277](https://github.com/bbatsov/rubocop/pull/2277): New cop `Style/FirstHashElementLineBreak` checks for a line break before the first element in a multi-line hash. ([@panthomakos][])
 * [#2277](https://github.com/bbatsov/rubocop/pull/2277): New cop `Style/FirstMethodArgumentLineBreak` checks for a line break before the first argument in a multi-line method call. ([@panthomakos][])
 * [#2277](https://github.com/bbatsov/rubocop/pull/2277): New cop `Style/FirstMethodParameterLineBreak` checks for a line break before the first parameter in a multi-line method parameter definition. ([@panthomakos][])
+* [#2329](https://github.com/bbatsov/rubocop/pull/2329): New option `EnforceMultiLineBraceChaining` for `Style/BlockDelimiters` cop enforces braces on a multi-line block if it's return value is being chained with another method. ([@panthomakos][])


it's -> its

jonas054 · 2015-10-24T08:32:56Z

I see. Then I think it's OK. But since the new parameter only makes sense for the line_count_based style, I think it's better to implement it as a third style, braces_for_chaining.

panthomakos · 2015-10-26T16:14:57Z

@jonas054 PTAL I've updated with your comments and implemented using the braces_for_chaining style.

jonas054 · 2015-10-27T11:19:21Z

lib/rubocop/cop/style/block_delimiters.rb

@@ -28,7 +28,7 @@ def on_block(node)
          if proper_block_style?(node)
            correct_style_detected
          else
-            add_offense(node, :begin) { opposite_style_detected }
+            add_offense(node, :begin) { unrecognized_style_detected }


Actually, those ..._style_detected methods don't do anything useful when there are more than two styles to choose from. So you can remove this block and the call to correct_style_detected.

jonas054 · 2015-10-27T11:19:56Z

I just had one more comment. And the Travis build is failing.

panthomakos · 2015-10-27T13:35:14Z

@jonas054 The failures were related to RuboCop rules. I have resolved all of them except for Class has too many lines. [167/147] Do you have a recommended way to resolve in this case? I don't see an obvious way to reduce the class size. I'm happy to refactor, but some guidance on direction would be helpful.

jonas054 · 2015-10-27T15:53:33Z

How to best refactor could be a long discussion, so for me it's OK if you just run ./bin/rubocop --auto-gen-config and add/commit the updated .rubocop_todo.yml. We can reduce the class length later IMO.

panthomakos · 2015-10-27T17:05:48Z

@jonas054 Ran the --auto-gen-config and that has resolved the class length issue.

jonas054 · 2015-10-27T21:11:48Z

👍 Looks good now.

bbatsov · 2015-10-28T06:32:25Z

Drop the . from the title of the commit message and rebase on top of the current master.

As per [the style-guide][style-guide], even though multi-line chaining is considered ugly and we should generally find ways to re-write the code, sometimes this simply isn't possible. Regardless, some code-bases would like to allow multi-line blocks to be used in method chains. We can also argue that multi-line chaining, when used, is better like this: ``` array.map { |x| x * 2 }.map(&:to_s) ``` than like this: ``` array.map do |x| x * 2 end.map(&:to_s) ``` This style, when selected, forces multi-line blocks to use braces if their return value is being chained with another method. I believe that this is a better option than the current default configuration which enforces the second of the two code examples. [style-guide]: https://github.com/bbatsov/ruby-style-guide

panthomakos · 2015-10-28T13:07:13Z

@bbatsov I always forget the period!

Period removed and branch rebased on master.

…aining New braces_for_chaining style for Style/BlockDelimiters.

bbatsov · 2015-10-28T16:28:05Z

👍 Thanks!

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from 48bb062 to de2bfc7 Compare October 20, 2015 04:49

panthomakos changed the title ~~EnforceMultiLineBraceChaining for BlockDelimiters.~~ EnforceMultiLineBraceChaining for BlockDelimiters Oct 20, 2015

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from de2bfc7 to 04ac153 Compare October 20, 2015 16:12

jonas054 reviewed Oct 24, 2015
View reviewed changes

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from 04ac153 to 7b6616e Compare October 26, 2015 16:11

panthomakos changed the title ~~EnforceMultiLineBraceChaining for BlockDelimiters~~ New braces_for_chaining style for Style/BlockDelimiters. Oct 26, 2015

panthomakos force-pushed the enforce-multi-line-brace-chaining branch 2 times, most recently from ecb7739 to 97cebf1 Compare October 26, 2015 16:14

jonas054 reviewed Oct 27, 2015
View reviewed changes

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from 97cebf1 to d9ba4c5 Compare October 27, 2015 13:32

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from d9ba4c5 to dfd0140 Compare October 27, 2015 13:36

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from dfd0140 to cf34a75 Compare October 27, 2015 16:51

panthomakos force-pushed the enforce-multi-line-brace-chaining branch from cf34a75 to b23c144 Compare October 28, 2015 13:06

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

Merge pull request #2329 from panthomakos/enforce-multi-line-brace-ch…

b0866cd

…aining New braces_for_chaining style for Style/BlockDelimiters.

bbatsov merged commit b0866cd into rubocop:master Oct 28, 2015

zzeni mentioned this pull request Feb 26, 2018

Style/BlockDelimiters breaks natural language of rspec matchers rubocop/ruby-style-guide#522

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New braces_for_chaining style for Style/BlockDelimiters. #2329

New braces_for_chaining style for Style/BlockDelimiters. #2329

panthomakos commented Oct 20, 2015

jonas054 commented Oct 22, 2015

panthomakos commented Oct 22, 2015

jonas054 Oct 24, 2015

jonas054 commented Oct 24, 2015

panthomakos commented Oct 26, 2015

jonas054 Oct 27, 2015

jonas054 commented Oct 27, 2015

panthomakos commented Oct 27, 2015

jonas054 commented Oct 27, 2015

panthomakos commented Oct 27, 2015

jonas054 commented Oct 27, 2015

bbatsov commented Oct 28, 2015

panthomakos commented Oct 28, 2015

bbatsov commented Oct 28, 2015

New braces_for_chaining style for Style/BlockDelimiters. #2329

New braces_for_chaining style for Style/BlockDelimiters. #2329

Conversation

panthomakos commented Oct 20, 2015

jonas054 commented Oct 22, 2015

panthomakos commented Oct 22, 2015

jonas054 Oct 24, 2015

Choose a reason for hiding this comment

jonas054 commented Oct 24, 2015

panthomakos commented Oct 26, 2015

jonas054 Oct 27, 2015

Choose a reason for hiding this comment

jonas054 commented Oct 27, 2015

panthomakos commented Oct 27, 2015

jonas054 commented Oct 27, 2015

panthomakos commented Oct 27, 2015

jonas054 commented Oct 27, 2015

bbatsov commented Oct 28, 2015

panthomakos commented Oct 28, 2015

bbatsov commented Oct 28, 2015