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.Empty #184

Merged
merged 11 commits into from
Jul 7, 2024
Merged

Conversation

RieBi
Copy link
Contributor

@RieBi RieBi commented Jun 14, 2024

A solution to issue xunit/xunit#1510, specifically the following part:

Optionally could do the same thing with Assert.Empty(...) and Assert.NotEmpty(...), mapping them to Assert.DoesNotContain(...) and Assert.Contains(...), respectively.

The analyzer and fixer apply to the following situations:
Assert.Empty(Enumerable.Where(expression))
and suggest them to change to:
Assert.DoesNotContain(Enumerable, expression)

@bradwilson hope to get feedback on it, if there's anything wrong with this

@RieBi
Copy link
Contributor Author

RieBi commented Jun 14, 2024

@RieBi please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@RieBi
Copy link
Contributor Author

RieBi commented Jun 14, 2024

@dotnet-policy-service agree

@RieBi
Copy link
Contributor Author

RieBi commented Jun 14, 2024

This doesn't seem to work :d

@bradwilson
Copy link
Member

bradwilson commented Jun 16, 2024

This doesn't seem to work :d

I'll see if I can chase down what's going on here.

Never mind, it seems to have done it's thing:

image

@bradwilson
Copy link
Member

I just wanted to stop by and let you know I'm not ignoring this PR; it's just currently prioritized behind a very large v3 rework that I'm in the middle of. I will get to this, I promise. 😄

@RieBi
Copy link
Contributor Author

RieBi commented Jun 23, 2024

I just wanted to stop by and let you know I'm not ignoring this PR; it's just currently prioritized behind a very large v3 rework that I'm in the middle of. I will get to this, I promise. 😄

It's fine, but I want to make sure I don't do anything wrong. Can I work on making another PR now, specifically regarding the issue xunit/xunit#1511 ? Would it be fine?

@bradwilson
Copy link
Member

Functionally everything looks great. The test coverage looks succinct and complete, and the code looks simple and easy to follow. Excellent job!

The only changes I'm going to push for you are coding style changes, all relatively minor and just me trying to keep things in the newer style that I want to push (a lot of older code is not up to date, so guessing what I'm typically looking for right now is probably impossible anyway). Mostly: I sorted namespaces, I restructured some of the tests so there was less redundant code (using more data-driven updates), and otherwise just made small stylistic changes.

I assume you intend to do the other half here as well (NotEmpty => Contains) as well before you want me to merge it?

@RieBi
Copy link
Contributor Author

RieBi commented Jul 7, 2024

I assume you intend to do the other half here as well (NotEmpty => Contains) as well before you want me to merge it?

No, you just said before that you were working on a large rework and I started wondering if the issue is still relevant. I assume it still is. Thanks for keeping your promise of not forgetting about this PR, though :D.

I might then start working on the other half in the near future, namely on the NotEmpty => Contains, but I'll make a separate PR for that, I think it would be better that way.

@bradwilson
Copy link
Member

Sounds good! Thanks again!

@bradwilson bradwilson merged commit a67a4c7 into xunit:main Jul 7, 2024
2 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