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

Add more specific variants of contain (continued) #1145

Merged
merged 39 commits into from Nov 17, 2019

Conversation

@danielmpetrov
Copy link
Contributor

danielmpetrov commented Sep 22, 2019

Hi,

I am attempting to finish off #962, since @mkolumb seems to be MIA.

There was quite the conversation around this change, and the agreed non-breaking API was described here - #962 (comment)

Now, I picked off where the last PR was at, and changed the API to be backwards compatible as described by @jnyrup, but I need some help on what would be the best way to get good failure messages for predefined delegates.

Closes #818 and Resolves #962

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Sep 22, 2019

Looks promising, although I don't yet see what you intend to do with StringContainmentConstraints

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Sep 23, 2019

@dennisdoomen I intend to remove StringContainmentConstraints.cs and StringWithOccurenceConstraint.cs once the PR is ready ready to merge. Those two classes are left over from the old breaking state of this change.

Now the problem I'm having is how to get good failure messages as in the original PR.

Any ideas welcome.

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Sep 23, 2019

Using reflection we can read whether the passed delegate method was one of the predefined by FA options and display appropriate message.

If the user passes a lambda instead, a more generic message will be shown, such as 'expected the number of times {expected} is contained within {actual} to match custom predicate'

Sounds good?

@danielmpetrov danielmpetrov marked this pull request as ready for review Sep 23, 2019
@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Sep 23, 2019

@dennisdoomen @jnyrup What do you think about this implementation?

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Sep 24, 2019

It most definitely looks promising.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 28, 2019

Is there anything left before we can do a proper review?

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Oct 28, 2019

@dennisdoomen As far as StringAssertions is concerned, there's nothing left to do.

From what I remember, last I was working on this I was concerned about the possibility of making this functionality reusable as suggested by
@jnyrup #962 (comment)

I believe this should be discussed before this PR is merged.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 28, 2019

The TimesConstraints looks pretty reusable to me. It's not tied to any specific assertion method other than using hard-coded strings in its FailWith method. But this is something we might be able to refactor later on. Just make sure it's internal.

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Oct 28, 2019

@dennisdoomen Fair enough.

One more question that I have is in which namespace should TimesConstraint, AtLeast, AtMost, MoreThan, LessThan, and Exactly reside? Currently they're in FluentAssertions.Primitives, but if they'll be reused for collections as well, should they be placed in the root FluentAssertions namespace?

Other than that, you guys are free to review this PR. 👍

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 28, 2019

One more question that I have is in which namespace should TimesConstraint, AtLeast, AtMost, MoreThan, LessThan, and Exactly reside

I'll take a look

Copy link
Member

dennisdoomen left a comment

Looks very good. For maintainability reasons, I have mostly minor comments. But I hope you can find the time to apply those suggestions.

Src/FluentAssertions/Primitives/StringAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/TimesConstraint.cs Outdated Show resolved Hide resolved
docs/_pages/strings.md Show resolved Hide resolved
Copy link
Collaborator

jnyrup left a comment

I haven't been giving this PR much attention lately, but it seems to getting in really good shape.
This function is a prime example of the (exhausting) prototyping-feedback loop when altering an API.

I found it somewhat difficult to review the tests and their seems to have been quite some rearrangement of the tests (unless it's just git diff confusing me).

Tests/Shared.Specs/StringAssertionSpecs.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/StringAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/StringAssertions.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/StringAssertionSpecs.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/StringAssertionSpecs.cs Outdated Show resolved Hide resolved
@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Nov 3, 2019

I found it somewhat difficult to review the tests and their seems to have been quite some rearrangement of the tests (unless it's just git diff confusing me).

It seems like the tests were rearranged in #962, which is the basis of this PR. That's something that confused me as well honestly.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

jnyrup commented Nov 8, 2019

@danielmpetrov Let me know if I should do the tedious work of untangling the re-arrangement of the tests.
I'll might have time this Sunday.

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Nov 9, 2019

@danielmpetrov Let me know if I should do the tedious work of untangling the re-arrangement of the tests.
I'll might have time this Sunday.

I'll give it a try 😅

@danielmpetrov danielmpetrov force-pushed the danielmpetrov:gh_818 branch from 68f1ba1 to ad7dd80 Nov 9, 2019
@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Nov 9, 2019

@jnyrup This is a git diff issue...

I've reverted the test file to match the one on master and then copy pasted all the new tests only nested under the contain and contain equivalent of regions of the test file, but the diff is still messed up.

@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Nov 9, 2019

@jnyrup Ok, good news. I cleaned up some duplicate tests, and the diff appears properly now. I guess the duplicated code snippets messed git up.

Copy link
Collaborator

jnyrup left a comment

Thanks for doing the re-arrangement and duplication removal!
It made reviewing the tests a piece of cake.

Src/FluentAssertions/Primitives/StringAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Primitives/StringAssertions.cs Outdated Show resolved Hide resolved
Tests/Shared.Specs/StringAssertionSpecs.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/AtLeast.cs Outdated Show resolved Hide resolved
@danielmpetrov

This comment has been minimized.

Copy link
Contributor Author

danielmpetrov commented Nov 12, 2019

It's your favorite failing test - When_the_execution_time_of_an_action_is_close_to_a_limit_it_should_not_throw 🎉

Please tell me if anything else needs work on this PR @jnyrup @dennisdoomen

@dennisdoomen dennisdoomen merged commit 97d737e into fluentassertions:master Nov 17, 2019
1 check failed
1 check failed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.