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

Replacing part of assertion #5

Closed
jnyrup opened this issue Oct 14, 2017 · 7 comments
Closed

Replacing part of assertion #5

jnyrup opened this issue Oct 14, 2017 · 7 comments

Comments

@jnyrup
Copy link
Member

jnyrup commented Oct 14, 2017

When verifying multiple assertions against the same object, you often combine them with .And.
Currently it seems the codefixer is not aware of this concept.

actual.Should().NotBeNull()
    .And.NotBeEmpty()
    .And.HaveCount(2);

is replaced with

actual.Should().NotBeNullOrEmpty();
@Meir017
Copy link
Member

Meir017 commented Oct 14, 2017

I'm not sure how to approach this feature yet. I opened an issue on the Roslyn project, maybe they have a helper for something like this

dotnet/roslyn#22698

@Meir017
Copy link
Member

Meir017 commented Oct 18, 2017

@jnyrup any idea what the code fix should be in the following case?

actual.Should().NotBeNull("collection should not be null")
    .And.NotBeEmpty("collection should not be empty")
    .And.HaveCount(2);

@jnyrup
Copy link
Member Author

jnyrup commented Oct 18, 2017

Are you referring to the problem about which becauseArgs to choose when merging NotBeNull and NotBeEmpty?

If it was my own code using FA I would just write:

actual.Should().NotBeNullOrEmpty("collection should not be empty")
    .And.HaveCount(2);

But of course the analyzer needs to be general.
How about this:

  • If both NotBeNull and NotBeEmpty has an becauseArgs suggest they could be merged, but without providing a codefix,
  • otherwise merge them and there is at most a single becauseArgs.

@Meir017
Copy link
Member

Meir017 commented Oct 18, 2017

Are you referring to the problem about which becauseArgs to choose when merging NotBeNull and NotBeEmpty?

exactly!

@jnyrup just came up with a way to do the replacement - here is a snippet https://gist.github.com/Meir017/48eef45a5b2525ce21ff12de0bae5dc0 how weird is this? ;)

I tried using DocumentEditor as mentioned in the stackoverflow post but after the second replacement if threw a NullReferenceException since it couldn't find the second node after the first replacement

@Meir017
Copy link
Member

Meir017 commented Oct 19, 2017

started work on the branch feature/replacing-part-of-assertion

@jnyrup
Copy link
Member Author

jnyrup commented Oct 19, 2017

Sounds great!
My colleagues are already eager to know when they can start using this tool.

@Meir017 Meir017 closed this as completed Oct 19, 2017
@Meir017
Copy link
Member

Meir017 commented Oct 19, 2017

@jnyrup 0.4.0 is up on NuGet now

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

No branches or pull requests

2 participants