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
Remove unecessary call to Subject.Should()
#2402
Conversation
Qodana for .NETIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2023.2.8
with:
upload-result: true Contact Qodana teamContact us at qodana-support@jetbrains.com
|
Pull Request Test Coverage Report for Build 6650591818
💛 - Coveralls |
@@ -1845,7 +1845,7 @@ public AndConstraint<TAssertions> NotBeEmpty(string because = "", params object[ | |||
|
|||
using (var scope = new AssertionScope()) | |||
{ | |||
Subject.Should().BeEquivalentTo(unexpected, config); | |||
BeEquivalentTo(unexpected, config); |
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.
What about adding methods called SubjectShouldXXXX(...)
which just calls XXX(...)
? It's just for the fluent readability 😉
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.
Or... 😉
Subject.Should().Subject.Should().Subject.Should().Subject.Should().Foo();
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.
Ok.. got it ;)
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.
Tounge-Twister ;)
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 just realized you weren't joking 🤦♂️
I'm fine with the current proposed changed, but also happy to add a AssertSubjectIsEquivalentTo
forwarding method.
Note that the three changed lines each call a different BeEquivalentTo
method.
Avoiding calling Should()
inside FA could help #2253
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 see what you are trying to do, but it doesn't read very fluent to me.
One can hardly disagree that To get some numbers on the table, this pattern gives 134 results grep -rInE --include=*.cs '^ *(return|=>)? *[A-Z][A-Za-z]+\(.*because, becauseArgs' | wc -l The vast majority are simple one-liner forwards to another overload. fluentassertions/Src/FluentAssertions/Primitives/ReferenceTypeAssertions.cs Lines 127 to 136 in 52622a0
fluentassertions/Src/FluentAssertions/NumericAssertionsExtensions.cs Lines 848 to 867 in 52622a0
Another pattern we use around a dozen places is to have non-public methods having Combined with #2403 the four ones calling |
Is this a third proposal (for #2403)? :) |
27cef01
to
dab1f3c
Compare
Unless
Subject.Should()
resolves to a another overload ofShould
than the initialsut.Should()
, callingSubject.Should().Foo()
instead ofFoo()
directly should not add any value, but on the negative side lengthen the stack trace in case the assertion fails.IMPORTANT
./build.sh --target spellcheck
or.\build.ps1 --target spellcheck
before pushing and check the good outcome