Fix 95: Ambiguous step reported when definition matches via more than one tag

[clrudolphi]

🤔 What's changed?

Modified the ProjectBindingRegistry to ignore duplicate matches when the Implementations were the same but differed only in the Scope of the match.

⚡️ What's your motivation?

Fixes #95

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

---

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

[clrudolphi]

Build failed one Spec about Completions that has nothing to do with this PR. I didn't change anything (at least I think not).

[304NotModified]

I've restarted the build, maybe it's a flaky test

[clrudolphi]

I've restarted the build, maybe it's a flaky test

Thanks. But that one failed too in the same place. It's a test related to editor completions.

I'll look at it tomorrow.

[clrudolphi]

I've restarted the build, maybe it's a flaky test

Thanks. But that one failed too in the same place. It's a test related to editor completions.

I'll look at it tomorrow.

Fixed the problem; it was my fault in how the test step definitions were getting defined.
I separated out to a distinct step/method for test setup for the multiple tag scenario.

[304NotModified]

@clrudolphi I see that your test is working (and red when the code change isn't there)

But I'm curious how it works, as MatchedStepDefinition.Implementation is of type ProjectBindingImplementation and that type doesn't have an equals/hashcode implementation nor is a record.

So this is deduplicated by reference? Isn't that tricky?

[clrudolphi]

So this is deduplicated by reference? Isn't that tricky?

Yes, GroupBy (in this situation) is identifying duplicates by reference equality. This works b/c the system does only ever create one instance of ProjectBindingImplementation per step/hook method. We got lucky that way.

We have a few options:

1. Leave it. It works but might break in the future if the instancing strategy changes.
2. Modify ProjectBindingImplementation to implement Equality
3. Add a custom IEqualityComparer<T> and pass that to the GroupBy function.
4. Modify ProjectBindingImplementation to be a record rather than a class.

-2 is probably OK but I'm not sure if we would inadvertently break something elsewhere that was depending upon reference equality.
-3 is easy and local (small blast radius).
-4 is doable as the existing class is clearly intended as a read-only DTO.

Do you see anything I may have overlooked? Any preferences?

[304NotModified]

I prefer 3, so it won't break in the future:)

[clrudolphi]

I prefer 3, so it won't break in the future:)

Implemented ( number 3, the IEqualityComparer<T>).