Semantic blocks

[iGEL]

I'm a long time user of the Weirich style and like that you don't enforce the line count thing on me. However, I'm not sure whether it's a good idea to attempt to detect whether a block is functional or not. I ran this on our project and got several things it got wrong:

CONCENTRATION.find { |end_date, _threshold|
  end_date >= Time.zone.today
}&.last || 0

was changed to the do ... end

@_document ||= Prawn::Document.new do
  font_families.update(
    "Courier" => {
      normal: "app/assets/fonts/source-code-pro/SourceCodePro-Regular.ttf",
      bold: "app/assets/fonts/source-code-pro/SourceCodePro-Bold.ttf"
    }
  )
end

was changed to { ... }

The Weirich style is imnsho close to impossible to enforce correctly (unless you leave many cases untouched, which makes it kinda useless) and isn't easy to understand for newer developers.

The line count style is just useless. The only advantage is it's wide usage.

We should decide, whether our affection for Jim Weirich and our wish to appear smart is worth the costs of this complication. If I'm honest to myself, I can't remember a case when I actually used this signal whether the block is functional or procedual.

To me it's similar to the double vs. single quotes thing for strings. Maybe it's even easier, because you don't need to care about the content of the block, while with strings, you might want to change the style depending whether you have quotes in it.

My preferences in order:

1. Always enforce braces
2. Don't enforce the block delimiters at all, so we can use the Weirich style
3. The Weirich style enforcement attempt or the line count style (With a bad feeling in my stomach)

[searls]

I understand and deeply appreciate that you shared your thoughts on this. Thanks also for providing specific examples—I can sorta guess/understand how the current implementation would lead

In truth, I'm similarly uneasy about how to proceed, and don't love any of the obvious options. Personally, after 10+ years of really appreciating this style, it'd be hard for me to give up. I think I need some more time to reflect on it. Obviously my first choice would be to invest in the SemanticBlocks cop where we can to get it closer to a reliably-correct state.

As it pertains to rubyfmt, @samphippen, what are your thoughts on this feature?

Anyone else have opinions?

[fables-tales]

I'm on team semantic blocks for sure.

I think we can just juice up the power of the analyser

[searls]

@samphippen you expressed some confidence yesterday we'd be able to find such "juice". Do you have any specific ideas in mind for making semantic blocks more readily detectible?

[iGEL]

I also would be interested to see that.

Especially since you don't allow any configuration (and I'm a big fan of that), mistakes would be very annoying here. If you want to enforce this style, I think you should only change the style if you highly confident about the change. Not enforcing this would be not optimal, but a lot better than enforcing it wrong.

A problem to me is also the for me unclear definition of a functional block. For example, would you define

date = Timecop.freeze(1.year.ago) { format_date(Time.now) }

as functional? How about this one?

customer = Timecop.freeze(1.year.ago) { create(:customer) }

Here Jim's definition:

• Use { } for blocks that return values
• Use do / end for blocks that are executed for side effects

[searls]

Yes, I understand and agree. I'm just not ready to throw in the towel that a better program than the current implementation could not exist that does a (much) better job of answering this question than the current set of heuristics.

As for Jim's definition, it sorta floated a bit over time depending on the context. Since there are no (real) statements in Ruby, only expressions, more-or-less all blocks will technically return a value. So the challenge in automating this check is that there's a question of authorial intent. (Jim pointed out that if the source of the called function is available, this is much less ambiguous, FWIW.)

[searls]

I'm about to push up a PR that adds a testing seam for the SemanticBlocks cop as well as a failing test for the case you pointed out above with the lonely operator.

I'll mention that I think this is working as advertised by changing to braces:

@_document ||= Prawn::Document.new do
  font_families.update(
    "Courier" => {
      normal: "app/assets/fonts/source-code-pro/SourceCodePro-Regular.ttf",
      bold: "app/assets/fonts/source-code-pro/SourceCodePro-Bold.ttf"
    }
  )
end

The rule, as StandardRB is interpreting it, is "does the invocation with a block return a useful function", and here it does (the document), not "does the terminal expression within the block represent the overall returned thing", which is (I agree) not quite knowable but also was not my impression of what Jim was going for with the rule

[cableray]

Also broken: rescue/ensure within blocks.

parsed_data = input_stream.map do |data|
  JSON.parse(data.read)
rescue JSON::ParserError
  nil
ensure
  data.close
end

Breaks when converted to curly braces.
(Maybe not the best example, but the idea is there)

[fdr]

Count me as a line-count person, though this seems the minority position. This level of complexity to deal with grammar issues on curly braces vs. hash literals is just not worth it.

[Cohen-Carlisle]

I have to say that I'm a line count person, too. It's so much simpler and also corresponds to using do...end for multiline and , do: ... for single line in elixir. And in my experience, in the wild it's much more common.

[iGEL]

This level of complexity to deal with grammar issues on curly braces vs. hash literals is just not worth it.

Which again would be an argument for always use braces, it's even easier than that. 😉 The line count is pretty stupid in my opinion, sorry to say. It's widely used, but is there any better reason than "This is how we've always done it"?

• It doesn't provide any new info, the number of lines is easy to grasp without it, too.
• Why should you change the braces/do...end when you change add or remove a line? Braces and do...end do have different precedence, so a change of style might require additional parenthesis on callers.
• Chaining looks stupid with do...end

some_value.map do |value|
  # some computation that's longer than one line
end.reduce(&+)

I've added always_braces to RuboCop and use it since then. And I like the result.

[Cohen-Carlisle]

[line count is] widely used, but is there any better reason than "This is how we've always done it"?

It does provide a nice consistency to the fact that end is used to delimit many other things that are multiline: classes, modules, ifs, cases, etc.

That said, always_braces is interesting.

I mostly don't want to get bogged down in whether a block is mostly functional or mostly procedural while formatting.

[fdr]

Which again would be an argument for always use braces, it's even easier than that.

To repeat myself, it's not though, because of grammar confusion around a hash parameter and block.

[fdr]

Okay, here's an example of a change in precedence order. Evidently, do has a lower precedence. Maybe I'm just writing my code wrong, but it needs more parenthesis now:

    e = assert_raises RuntimeError {
      a < b
    }

Is not the same as:

    e = assert_raises RuntimeError do
      a < b
    end

The former complains:

       undefined method `RuntimeError' for #<RSpec::ExampleGroups::SoftwareVersion:0x00007fcafda9cc50>

Now maybe parenthesis are the way to go, so if --fix ever learns a thing, it has to learn it a bit carefully.

[iGEL]

Evidently, do has a lower precedence. Maybe I'm just writing my code wrong, but it needs more parenthesis now

Exactly, but I don't get how this is an argument for the line count. This doesn't work either:

e = assert_raises RuntimeError { a < b }

And if for example you extract your long block to a method, you even need to add parenthesis:

# before
assert_raises RuntimeError do
  # some
  # code
  # to
  # extract
end

# after
assert_raises(RuntimeError) { extracted_method }

Also, most of the code I see these days even with the line count style, uses almost everywhere parenthesis, even if it's in most cases not required.

Same for your other argument:

To repeat myself, it's not though, because of grammar confusion around a hash parameter and block.

This only makes sense to me, if you always use do...end. Also, for me that was never a problem, a hash is easy to spot thanks to the keys:

{
  a: 1,
  "b" => 2
}
# vs
{
  a(1)
  b(2)
}

And personally, I add spaces inside of blocks, but not for hashes:

{a: 1, b: 2}
# vs
{ a(1) }

[pftg]

Is it possible to run some poll, because semantics blocks are great, but have not found popularity for the last 10 years?

I have started to help other OSS to use standard gem, and got some feedback/block when I'm showing this way.

So in order to get wide usage of the standard need that other gem owners start to use it, but this rule is one of the rules breaking and will stop them to review it at all as a replacement to custom rubocop config.

I think it's possible to disable it, but then the configuration-less feature will be breaking.

[dnsco]

I just opened #248 – I support the intention of the rule, but it seems really hard to get enforcement on it to be correct, and the cases where it's wrong are quite bothersome.

Also, thanks for the great library everyone! I love not having to think about these kinds of issues the majority of the time!

[jmkoni]

I've done a bit of testing and I think all the examples that were previously not working seem to be working now. For example:

e = assert_raises RuntimeError {
      a < b
    }

parsed_data = input_stream.map do |data|
  JSON.parse(data.read)
rescue JSON::ParserError
  nil
ensure
  data.close
end

foo =  blah.map { |e|
  begin
    #…
  rescue
  end
}

are all unchanged when I run standardrb --fix. If there is a new example that is not working, please let us know.