Don't spend CPU finding the same node twice

[bquorning]

As mentioned in #955 (comment)

---

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]

I wonder does it change autocorrect with multiple offending nodes, or the first_offending_node was recalculated after each autocorrelation?

[bquorning]

I don’t think I understand your question. Could you add an example?

[Darhazer]

RSpec.describe User do
        let(:params) { foo } # first_offsending_node
        let(:bar) { baz } # sibling

        subject { described_class.new }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Declare `subject` above any other `let` declarations.
      end

Previously it would always move before the first offending node. Now in each iteration, it would move before the current offending node. Unless those are re-evaluated. I added the test and it's not broken.
And the reason for this is that we break after the first offending node, so sibling and first_offending_node are always the same. I missed that detail in the review

[Darhazer]

Perhaps the readability of the method could be improved as well

         def check_previous_nodes(node)
-          node.parent.each_child_node do |sibling|
-            if offending?(sibling)
-              msg = format(MSG, offending: sibling.method_name)
-              add_offense(node, message: msg) do |corrector|
-                autocorrect(corrector, node, sibling)
-              end
+          offending_node(node) do |offender|
+            msg = format(MSG, offending: offender.method_name)
+            add_offense(node, message: msg) do |corrector|
+              autocorrect(corrector, node, offender)
             end
-
-            break if offending?(sibling) || sibling.equal?(node)
           end
         end
 
+        def offending_node(node)
+          node.parent.each_child_node.find do |sibling|
+            break if sibling.equal?(node)
+
+            yield sibling if offending?(sibling)
+          end
+        end
+

[bquorning]

I like where this is going.

I tried replacing the last block body with

break if sibling.equal?(node)
break unless offending?(sibling)

yield sibling

and all tests still pass. if feels wrong. Do we need another test case, or is it ok to rewrite yield sibling if offending?(sibling)?

[Darhazer]

It could be some def or any other ruby code actually that goes before the subject

Edit: I see @pirj actually already answered. Should read all my mails before replying

[bquorning]

Fyi @pirj calling each_child_node(:block) fails for the spec containing let(:user, &args[:build_user]).

[Darhazer]

As far as I remember, you can pass multiple node types, like each_child_node(:block, :blockpass)

[bquorning]

I tried that but didn’t make it work :-)

[Darhazer]

Yeah, the block_pass is an argument, and the node itself is a send.

s(:send, nil, :let,
  s(:sym, :user),
  s(:block_pass,
    s(:send,
      s(:send, nil, :args), :[],
      s(:sym, :build_user))))

You can do each_child_node(:block, :send). Not sure if is worthy

[pirj]

Please feel free to ignore my observation notes, they don't relate to the subject (no pun intended) directly.
I'll file a tech ticket - we can address separately.

Merge when you see fit, looks good.

[pirj]

Observation:
any? it feels weird. first should work just fine. Can't imagine subject(...) { } being defined in an iterator, neither I'm aware of any special syntax to wrap subject, e.g. isolated { subject(...) { } }. first should work fine for most cases.

[pirj]

I guess it's break unless offending? doing a bad job. Shouldn't it be next?
Still, it's something worth adding a red spec. Maybe something like:

describe do
  def helper_method # `break unless offending?(sibling) would break here and skip subsequent offences
  end

  it { } # offence!

  subject(:lost_in_space) { ... }
end

[pirj]

node.parent.each_child_node

👍

Observation:
Wondering if each_child_node accepts a type (each_child_node(:block)), we could narrow down our search to only let/hooks/examples.

[pirj]

Observation:
Shouldn't || example_group?(node) be added here?
I can think of:

describe do
  describe do
    ...
  end

  subject(:i_am_offended_by_a_preceeding_example_group_where_i_am_a_full_fledged_subject) { ... }
end