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 ForConstraint to AssertionScope to open up OccurenceConstraint for usage in custom assertion extensions #1341
Add ForConstraint to AssertionScope to open up OccurenceConstraint for usage in custom assertion extensions #1341
Conversation
…traint for usage in custom assertion extensions
@@ -678,10 +678,10 @@ public AndConstraint<TAssertions> Contain(string expected, OccurrenceConstraint | |||
int actual = Subject.CountSubstring(expected, StringComparison.Ordinal); | |||
|
|||
Execute.Assertion | |||
.ForCondition(occurrenceConstraint.Assert(actual)) | |||
.ForOccurrences(occurrenceConstraint, actual) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ForConstraint
will work better, but I'm not 100% sure. I even ask Twitterverse for some guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that guys in Twitter aren't actually getting the context for these concepts. IMHO ForConstraint
will work just fine, it's correct in technical sense.
febeb61
to
3baa7bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'm repeatedly amazed about how much FA users apparently extend the library.
It would be nice having a test that adds a custom OccurrenceConstraint
to exemplify the purpose of ForConstraint
for library consumer.
Improves library extension interface as requested in #1281.
Some equivalent
ForCondition
usages are replaced withForOccurrences
(StringAssertions.Contain
andStringAssertions.ContainEquivalentOf
), and as methods containing them covered with unit test, I considerForOccurrences
covered enough as well.The existence of
{expectedOccurrences}
placeholder is mentioned in documentation comment forForOccurrences
method. I don't think it should be inFailWith
doc comment as it would make it too large and hard to read.IMPORTANT
AcceptApiChanges.ps1
/AcceptApiChanges.sh
.