Alter binding source processing to no longer look for BindingAttribute

[Code-Grump]

🤔 What's changed?

This changes the binding source processing to no longer look for the presence of a Binding attribute on the class. Now, simply using any of the binding attributes (step bindings, hooks, etc) is sufficient to trigger a class being treated as binding type.

⚡️ What's your motivation?

This change is in-keeping with most test frameworks to no longer require marker attributes at the class level, simplifying user code somewhat.

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

The changes made to the processor have been done in a way that should maintain backwards-compatibility at the contract. I would appreciate this being scrutinised for potential problems.

📋 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.

[304NotModified]

I’m curious about the performance impact — there could be many methods involved. In time and memory.

Creating a list for every type at least adds memory pressure, and reflection is relatively slow.

P.S. There’s a typo in “candidate”.

[Code-Grump]

This definitely adds a cost for us - the system is written to produce a lightweight model layer between the actual reflection and the processing of binding methods, and without restructuring it more significantly, there's going to be allocations we could avoid.

However, I'd simply make the case that it's happening on a scale that's hard to perceive for most use-cases. This is the model that NUnit has used for a long time and that xunit has always used. If we'd like to make it as efficient as theirs, we'd need to do something more breaking in our APIs to refactor the component to yield types more efficiently.

[304NotModified]

Good one about xUnit and NUnit. Im don't know if they use some kind of cache.

[Code-Grump]

I'd like to draw attention back to this PR. @gasparnagy @clrudolphi is there anything you would like to know about this? Is it worth pursuing?

[clrudolphi]

What should be done with the documentation and templates in the VS Extension?

Do we put a 'deprecated' marker on the attribute?(warning)

[Code-Grump]

What should be done with the documentation and templates in the VS Extension?

I think if we want to do this right, we'd want to update the documentation to clarify the situation: before vNext the attribute is required; after upgrading to vNext, these attributes should be removed.

We'd also want to update the templates associate with vNext to no longer emit the attribute. That one is going to be annoying for people who update the package more frequently than the extension, but they're free to delete the attribute and then complain here about it. I don't see an obvious way to head that off.

Do we put a 'deprecated' marker on the attribute?(warning)

I think this would be a good move - mark the attribute with [Obsolete]. It'll flag all the places in our own tests that will need to be updated.