Extract some shared logic to BlankLineSeparation

[bquorning]

In 0289cd8, a bunch of logic was extracted into BlankLineSeparation module. In this PR I extract another bit.

Please help me with the naming of the method. I am not sure if check_missing_separating_lines is the best name.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• [-] Added tests.
• [-] Updated documentation.
• [-] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

[Darhazer]

How about missing_separating_line_offense
I also wonder if we can have a message method in order not to initialize a message without having an offense.

[Darhazer]

Another option for a cleaner API would be

missing_separating_line_offsense(node) do |message|
  message << format(....)
end

this way it's obvious that the offense is something that could be registered. But I don't quite like providing the argument just for the sake of modifying it with the message, and yielding or returning the message out of the block is just non-intuitive API

Anyway, those are just a couple of suggestions

[pirj]

Wondering if the message logic can be extracted somehow as well, at least the MSG itself.

[pirj]

Will it work as

example_group?(node) do |example_group|
  msg = format(MSG, example_group: example_group.method_name)
  check_missing_separating_lines(example_group, message: msg)
end

[pirj]

Same above for EmptyLineAfterExample and below for EmptyLineAfterHook.

[bquorning]

That would be neat, but we’d have to add a capture in the right place in RuboCop::RSpec::Language::SelectorSet. Something to explore in a separate PR, maybe?

[Darhazer]

It still could be

example_group?(node) do
  msg = format(MSG, example_group: node.method_name)
  check_missing_separating_lines(node, message: msg)
end

but then I would remove the ? from the name. Still, something that could be explored in a separate PR

[pirj]

Up to you. Looks good as is.
There's always an indefinite space for improvement.

[bquorning]

How about missing_separating_line_offense

I like it.

I also wonder if we can have a message method in order not to initialize a message without having an offense.

https://github.com/rubocop-hq/rubocop/blob/v0.88.0/lib/rubocop/cop/base.rb#L86-L91 says “Cops are discouraged to override this (#message); instead pass your message directly”.

I changed the logic to yield the method name of the offending node, to let the calling code generate the error message. Please review again.

[pirj]

I hate this part of Ruby where an included module can't access including class's constants directly. Just can't figure out the reasoning behind this behaviour.

There's a way around, though. If you decide to further reduce the amount of code in cops by pushing it to the module, a self.class::MSG will have the access to cop's MSG rather than module's. I'm on the fence with this though, as constructs like that always wake alarm my active thinking and get me out of the flow. On the other hand, it would be possible to get rid of yield, and instead of passing a block with a single format call, it would be possible to pass the argument name directly, i.e.:

missing_separating_line_offense(node, :hook)

or even make an agreement to name the parameter in MSGs (or don't use a named param):

        MSG = 'Add an empty line after `%<name>s`.'
# or
        MSG = 'Add an empty line after `%s`.'

and in the cops then:

missing_separating_line_offense(node)

Still, with a self.class::MSG on the other side of the scales, it's a tradeoff. Pick a solution that appeals to you most.
I'm pretty happy with how it is now.

[pirj]

Please merge at will 👍