Introduce CombinatorialClassDataAttribute #99
Conversation
There was a problem hiding this comment.
Introducing this interface is technically a breaking change since the Values property on the attributes has been replaced with the GetValues method.
|
|
||
| namespace Xunit; | ||
|
|
||
| #if NETSTANDARD2_0_OR_GREATER |
There was a problem hiding this comment.
Generic attributes cannot be reflected over (with standard .NET reflection APIs) on .NET Framework.
If we are to add this attribute, I believe we should only declare it when targeting .NET.
It appears the value-add of this class is to add a generic type constraint. We could achieve a similar guard for the non-generic attribute type (and drop the need for this generic one) by adding an analyzer.
There was a problem hiding this comment.
Yes, I agree. The usefulness of generic attributes is rather slim. I've tried to come up with good use cases, but I always seem to fall back on ones using Type instead.
There was a problem hiding this comment.
To be clear, are you suggesting we drop the idea of generic attributes, or should I change the condition?
There was a problem hiding this comment.
I'd go either way, but I'm leaning toward "less is more", so I'd mildly prefer we drop the generic attributes.
Then, if we want to add compile-time enforcement of the types provided, we can add an analyzer. But I'm guessing that's not worth the effort if a runtime exception will get the point across soon enough.
There was a problem hiding this comment.
Okay, I'm fine dropping the generic attributes as well. However, the PR and branch names should be changed then, or what do you think?
There was a problem hiding this comment.
I don't care about the branch name. But the PR title can be edited, sure.
There was a problem hiding this comment.
Alright! I've removed the two generic attributes and renamed the PR.
siewers
changed the title
|
I noticed the readme might need an update. Should I do that, or would you instead do that yourself? |
|
By all means I'd welcome you updating the docs. But don't update the README for the v1.7 branch, since no one reads that. So if you can update the docs after that's been done, I'd appreciate it. |
Fixes #95 and introduces support for combinatorial class data.
This PR is all over the place, but I think it provides improvements overall.
There are many tests that could or should be cleaned up. I tried my best to keep things together, but the mix of generic attributes and different ways of accessing data makes it hard to organize. Any ideas are welcome.
I also resolved some rebasing conflicts with my branch, which seems fine since all tests are still passing.
However, I might have missed some cases, in which case more tests should be added.
It might also be related to some cleanup done recently, which I haven't kept up with - I'll fix all of those if needed.