BlockDelimiters cop uses semantic style.

[fhwang]

The default option for Style/BlockDelimiters, line_count_based, simply
counts the number of lines in the block to determine whether to use
braces or do/end. These leads to superficial visual consistency, but
actually contradicts the semantic rule, which says that the choice of
braces vs do/end should depend on whether the block's purpose is
functional or procedural, i.e. whether it returns a value or has side
effects. When possible we should be using static analysis to enforce
conventions around a programmer's intent, which is more critical
information than how many lines a piece of code has.

The semantic option, used here, is implemented with a relatively
simple check to see if the return value of the block is assigned,
receives a method call, or is the last thing inside of an outer scope
that will thus return it. General consensus on the PR is that, although you
can probably find some edge-cases that will lead to counterintuitive
rule interpretations, this is a good-enough implementation.

Implementing PR: rubocop/rubocop#1731
Jim Weirich's initial request for the feature (RIP):
rubocop/ruby-style-guide#162

[fhwang]

For an example of how this would affect checks, here's this new config run against Agent: https://codeclimate.com/repos/5f40225e1958b90176003abd/pull/95

[nporteschaikin]

Works for me — really enjoyed your the original PR message I read yesterday 😅 , although this one is good/descriptive too.

[larkinscott]

👍

[wfleming]

I'm copy-pasting part of a comment from https://github.com/codeclimate/agent/pull/95#issuecomment-701675091, where this change was originally proposed, to aid others reviewing this in making a judgement call:

Am I right that the key distinction this config is making is whether the result of the method the block was passed to is itself an argument to a method, or the rval of an assignment, or the callee of a method in a chain?

E.g.:

• puts( map { |x| x } ) is braces because the result of map is passed to puts
• map { |x| x }.select do |x| x > y end - the map needs to be braces because the result is chained with select, but select is do because the result isn't used in that scope.
• z = map { |x| x }.select { |x| x > y } - now select is also braces because the result is the rval of the assignment to z
• def foo; map do |x| x end; end is do/end because the result of map isn't used within this scope (it might be elsewhere, e.g. you could call foo.inspect somewhere else)

---

That being said, I'm personally on the fence about this change, so I'm going to recuse myself from either approving or voting "nay". On the pro side, I see the appeal of using the block delimiters as a semantic marker of how the value is being used. On the con side, I generally don't like style rules where making 1 change forces you to make other changes.

E.g. if you had code like

def foo
  c.map do |x|
    f(x)
  end
end

And you realize you need to chain a select or something on there:

def foo
  c.map { |x|
    f(x)
  }.select do |x|
    x.relevant?
  end 
end

A small diff has turned into a bigger diff - and this is a very small toy example. In a bigger example, if the map line is further away from the select, the diff is going to get noisier (isolated lines where the only change is { -> do or vice versa). For me, good style rules are rules that minimize the chance that changing 1 line forces the developer to change more lines, or forces the reviewer to read a larger diff (the former can often be automated, the latter really can't - even if the reviewer isn't "reviewing style changes" because that's automated, the reviewer still needs to read the diff).

FWIW this is also why I don't like aligned tokens. Unlike aligned tokens, though, I do see the appeal of the "pro" side here. But I'm not convinced the "pro" is helpful enough to override the cons - in a language like Ruby I can't really think of a time when it really mattered to me if the result of a block was part of a chain or was assigned to a var.

---

That's my personal take, but changes here affect the whole team, and the team is now larger than it has been at other times we've debate style rules. I suggest this might be a good time to get input from more of @codeclimate/developers, and using the 👍 , 👎 reaction on the PR description itself also might be good to do here to gauge overall interest.

[fhwang]

@wfleming The key condition can be found here: https://github.com/rubocop-hq/rubocop/blob/433babd9c642f14121067ab85fb446b15dc313f2/lib/rubocop/cop/style/block_delimiters.rb#L341

It considers a block to be functional if the return value is used, or is the return value of the enclosing scope. So to take the examples you listed:

• puts( map { |x| x } ): This should be braces, because the result of map is passed to puts
• map { |x| x }.select do |x| x > y end: If this statement is inside a method where it's the last line, then this will be an issue because the rule will assume that the caller of the method needs the value returned by select. Which would seem to be the only intent of such a statement.
• z = map { |x| x }.select { |x| x > y }: Also needs to be braces, yes, because of the z assignment
• def foo; map do |x| x end; end: This would be incorrect, as the rule will assume that the caller of foo cares about the value returned by map.

Relatedly, the example you gave of

def foo
  c.map do |x|
    f(x)
  end
end

would be invalid on its own, without the later lines being added, so the concern about adding noisy changes wouldn't apply in that example.

Personally, I rely on this semantic distinction on blocks a lot, because there are lots of micro-cases where it's useful to define a method in a purely functional style. Side effects are obviously super-useful, which is why we're not all LISP programmers, but it's also useful at times to be able to communicate when a block of code is expected to have no side effects at all.

[wfleming]

Ah, I didn't notice a description of the "return value" condition. Ok, that does encourage me more. I'd expect like 90% of our usage to be braces, then, and a glance at the quality diff kind of bears that out. I still would like to see a couple more 👍 votes from the team before merging, but since everyone else's feedback has been positive so far, I think you'll probably get those votes.

[kurko]

I agree and prefer this way. When I joined CC I wrote a multi-line block with curly braces due to muscle memory and was asked to change it to do end because it was multiline, so I'm personally used to functional blocks as criteria for curly braces or not.

I think that enforcing do end for multiline block appeals only aesthetics, it doesn't add any semantic value (e.g it's do end, therefore it's multiline) because we know it's multiline just by looking at it.

By making blocks use { } when functional, we gain the semantic signalling at a glance, and we know right away that the return value is of importance. That semantic benefit is worth it IMO, as it now has a function.

@wfleming following your comment about 1 line changing 2 in git, perhaps we should also ditch trailing dots and go with leading ones, as per https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Layout/DotPosition 🤔 (for the annoying case of commented lines breaking Ruby, it has been fixed in Ruby 2.7)

ps. By functional here it means that the return value is used somewhere.