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 ValuesUsageAnalyzer. #570

Merged
merged 19 commits into from
Aug 29, 2023
Merged

Create ValuesUsageAnalyzer. #570

merged 19 commits into from
Aug 29, 2023

Conversation

andrewimcclement
Copy link
Contributor

Aiming to resolve #52 , though I haven't actually shared any code at this point. Refactoring suggestions are very welcome.
All tests are duplicated from TestCaseUsageAnalyzerTests. AnalyzeWhenArgumentPassesNullToValueType demonstrates [Values(null)] reporting a slightly unhelpful type to the user - fixes/suggestions are welcome.

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It is a good start.
Please add the documentation .md file so the tests can pass.
If you run the test locally you get a skeleton file you can rename and update.

I'm not sure how much code is sharable between the two analyzers, except for the tests to see if a parameter can be assigned from the supplied value.
The analyzers are different that one has multiple values for one parameter, the other a single value for each parameter.

@andrewimcclement
Copy link
Contributor Author

andrewimcclement commented Aug 14, 2023

@manfred-brands I used NUnit2048 as it was simply the next number in the list without skipping to the 3000s, but should this be in the 1000s? I'm not really sure of the overall scheme.

@andrewimcclement
Copy link
Contributor Author

Now using NUnit1031 for consistency with other structural issues.

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggested changes to documentation.

documentation/NUnit1031.md Show resolved Hide resolved
documentation/NUnit1031.md Show resolved Hide resolved
documentation/NUnit1031.md Outdated Show resolved Hide resolved
Andrew McClement and others added 18 commits August 19, 2023 12:08
… not yet do anything but basic structure is in place.
…ject?[] for the wrong type for [Values(null)] - can we do better?
Moved AdjustArguments to re-usable extension method.
Added GetAdjustedArgumentSyntax to get proper argument from explicit array parameters

Added unit tests for explicit array parameters
Added unit tests for nullable
Added unit tests for explicit array parameters
Added unit tests for nullable
@manfred-brands
Copy link
Member

@mikkelbu As I also made changes, I would appreciate your review when you have time.

@mikkelbu
Copy link
Member

@manfred-brands I'll take a look at it now. And thank you both for all the work

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I only found a couple of nitpicks.

Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>
@manfred-brands manfred-brands merged commit d5c0fea into nunit:master Aug 29, 2023
3 checks passed
@andrewimcclement andrewimcclement deleted the aim/values-usage-analyzer branch August 30, 2023 22:57
@mikkelbu mikkelbu added this to the Release 3.7 / 2.7 milestone Sep 8, 2023
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.

Reuse TestCaseAttribute logic for ValuesAttribute
3 participants