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/fixer for Assert.NotEmpty #185

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

RieBi
Copy link
Contributor

@RieBi RieBi commented Jul 21, 2024

So this is intended to solve the other part of xunit/xunit#1510, but I've got a few questions to @bradwilson before doing it.

First of all, I didn't realize when I was doing first half that they are almost the same, so I wonder how best would be to do it. I assume it would be fine to change the existing classes to account for both descriptors, but want to make sure if that would be fine.

As a side thing, this method name assumes that the analyzer does not trigger on strings, while in fact it does trigger. So, is that just a name thing and in fact it should trigger, or it is not intended to be triggered on strings and should be changed as well?

These are the things I want to clarify, and afterwards I think I'll work on finishing resolving the issue.

@bradwilson
Copy link
Member

I assume it would be fine to change the existing classes to account for both descriptors, but want to make sure if that would be fine.

Absolutely!

And apologies if the code looks different; I finally got motivated to standardize the many years worth of code style drift, motivated by discovering /* lang=c#-test */ syntax highlighting. 😄

As a side thing, this method name assumes that the analyzer does not trigger on strings, while in fact it does trigger.

You are correct! In my attempt to fix up all the naming (and I'll note sheepishly I missed the test above it), I put the wrong text there. It is intended to trigger. I have just pushed up name changes for both methods. 🙇🏼‍♂️

@RieBi RieBi marked this pull request as ready for review July 23, 2024 10:03
@RieBi
Copy link
Contributor Author

RieBi commented Jul 23, 2024

This is it, I guess.

If the tests are correct, which they should be, it finds the following situation:

Assert.NotEmpty(Enumerable.Where(expression))

and suggest to change to:

Assert.Contains(Enumerable, expression)

I'm looking forward to receive the feedback on it, if there are any issues with my implementation.

@bradwilson
Copy link
Member

Implementation looks great! My only changes are for me being nit-picky about spacing, and I'm going to slightly reword the messages so they match the others.

Thanks again!

@bradwilson bradwilson merged commit 0122310 into xunit:main Jul 24, 2024
5 checks passed
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