Update RuboCop and use children matching

[pirj]

Children matching was introduced in RuboCop 0.68.0.
This is a conservative update of RuboCop dependency to 0.68.1 (that has some minor bug fixes compared to 0.68.0).

A couple of our cops were updated to use children matching.
It allowed to convert iterations with that new pattern syntax, and reduce the amount of code.

Focus cop is intentionally left unchanged, there are improvements for it in #777 that are making use of the new syntax as well.

As per #777 (comment) and #773 (comment).

It seems I just had a stroke of syndrome.

---

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.
• [-] Added an entry to the changelog 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).

[pirj]

Appreciate a hint how to collapse the two above node patterns into one with % syntax.

Tried replacing them with:

        def_node_matcher :aggregate_failures?, <<-PATTERN
          (block {
              (send _ _ <(sym :aggregate_failures) ...>)
              (send _ _ ... (hash <(pair (sym :aggregate_failures) %) ...>))
            } ...)
        PATTERN

and using as aggregate_failures?(node, true), aggregate_failures?(node, :true), and aggregate_failures?(node, 'true') - none of those variants worked.

[Darhazer]

How about just capturing the argument, then in the block that checks for aggregate_failures you just need to check whether the node matched and if there is a captured value - it is true?

[pirj]

It turns out to become quite hacky for two reasons:

• I have to also capture from the other {} branch
• in the latter branch it's expected to be a boolean, for the former it will be a symbol

so I have to check it like that:

def_node_matcher :aggregate_failures?, <<-PATTERN
  (block {
    (send _ _ <$(sym :aggregate_failures) ...>)
    (send _ _ ... (hash <(pair (sym :aggregate_failures) $_) ...>))
  } ...)
PATTERN

def example_with_aggregate_failures?(example_node)
  node_with_aggregate_failures = find_aggregate_failures(example_node)
  return false unless node_with_aggregate_failures

  aggregate_failures = aggregate_failures?(node_with_aggregate_failures)
  aggregate_failures.true_type? || aggregate_failures.sym_type?
end

it works, but it's hard to understand.
Tried reading RuboCop's own usages of %-syntax, still can't understand how it works, and if it's possible to pass it an _ with no success.

I'd rather keep this as is, it's easier to spot the redundancy like that and address in the future. WDYT?

On the other note, I added a line to the changelog, but there were no user-facing changes, so just mentioning that RuboCop version dependency has been updated.

[pirj]

@marcandre Do you happen to know if it's possible to pass a boolean or an any as a node matcher argument (to substitute %)? I think I tried with booleans and failed.

[marcandre]

Super interesting question. Turns out you won't be able to because the check is that node == your_arg. We should revert that order for more flexibility, so it is possible to define an any matcher (ANY = Object.new.tap {|o| def o.==(_); true; end})
I'm thinking of something even a bit nicer, PR incoming

[pirj]

Got it, thanks!
Please keep in mind it's a pretty narrow case with just one potential usage I know if. Other cops here in rubocop-rspec using % provide symbols, and it's quite sufficient for them.

And this cop works as well, I just keep it somewhere in my mind's dirty corner that it might be improved.

[marcandre]

It's a simple change, but so much more powerful https://github.com/rubocop-hq/rubocop-ast/pull/31. You could have ANY = ->(_) { true } and pass that.

[pirj]

Two node patterns easily collapsed into one with this new generic parameters #930

[Darhazer]

You also need to add a changelog entry

[pirj]

@bquorning @Darhazer @dgollahon Is there anything that needs to be addressed here?

Speaking of aggregate_failures?/aggregate_failures_present? - I would rather keep two very similar patterns in hope to collapse them into one in the moment of enlightenment rather than to build support code around an improperly collapsed one to work around the difference in its application.

[pirj]

Fixed CHANGELOG conflict.

[bquorning]

@dgollahon Would you have time to review this PR?

[pirj]

@dgollahon Appreciate if you could take a look. Those changes are very similar to what you did in #777.

[bquorning]

I’m inclined to just merge this – the changes look fine, as far as I can see, and I’m pretty confident that the spec suite covers what I didn’t see :-)

[bquorning]

❤️ It’s so concise! I just hope people (we) will find it readable in the future.