Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create analyzer for constructors of types derived from FactAttribute #175

Merged
merged 7 commits into from
Jan 8, 2024

Conversation

JamesTerwilliger
Copy link
Contributor

@bradwilson Proposed analyzer and fixer for issue xunit/xunit#2120

Looks for constructors on classes that inherit from FactAttribute and makes sure that there are no internal or protected internal constructors. I make the assumption that if for some reason there are private or protected constructors that those are there for a reason.

Certainly open to other logic as to what visibility modifiers are "OK" here, as well as whether it's better to mark the class itself or the usage of the class inside an attribute.

@bradwilson
Copy link
Member

I think the logic probably needs to go like this:

Inspect the Fact (or Fact-derived) attribute and discover what constructor the user is calling, and if that constructor is not public, then raise the analyzer.

The mere presence of an internal constructor is not necessarily a problem, unless the developer tries to use it for their test. It's also possible (likely, in the case of Fact and Theory) that the Fact-derived attribute doesn't have any constructors, so that scenario also needs to be supported.

@bradwilson
Copy link
Member

Thoughts about adapting this? Are you still interested in doing this @JamesTerwilliger?

@JamesTerwilliger
Copy link
Contributor Author

Thoughts about adapting this? Are you still interested in doing this @JamesTerwilliger?

Apologies, I've been underwater the past few weeks. I can still take this on. With the suggested approach, we can certainly flag the uses of the constructor, but I'm not sure we'd also be able to have a fixer. I'll at least get the analyzer in place without a fixer and start there.

@bradwilson
Copy link
Member

I don't think a fixer is necessary.

@JamesTerwilliger
Copy link
Contributor Author

I don't think a fixer is necessary.

@bradwilson In that case, I think this is ready for review now

@bradwilson bradwilson merged commit f38ec16 into xunit:main Jan 8, 2024
5 checks passed
@bradwilson
Copy link
Member

Thanks!

@JamesTerwilliger JamesTerwilliger deleted the jamest/public branch February 7, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants