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

Extend BooleanAssertions with Implies #1886

Closed
marcrocny opened this issue Apr 11, 2022 · 21 comments · Fixed by #2074
Closed

Extend BooleanAssertions with Implies #1886

marcrocny opened this issue Apr 11, 2022 · 21 comments · Fixed by #2074
Assignees
Labels
api-approved API was approved, it can be implemented

Comments

@marcrocny
Copy link

Description

Add logical implication to the available boolean assertions.

That is, a.Should().Imply(b) asserts that a → b.

This would be useful in asserting, for example, that object equality implies hash-code equality without additional logic and a better built-in "because".

Complete minimal example

Looking for something like this.

[DataTestMethod]
[DataRow(false, false)]
[DataRow(false, true)]
// failure case: [DataRow(true, false)]
[DataRow(true, true)]
public void A_Implies_B(bool a, bool b) => a.Should().Imply(b);

Note this is logically equivalent to !a || b.

Additional Information

Also volunteering to add this.

@jnyrup jnyrup added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 11, 2022
@dennisdoomen
Copy link
Member

Sorry, I don't get it.

@jnyrup
Copy link
Member

jnyrup commented Apr 12, 2022

Which part don't you get?

a → b is called implication and is read "a implies b" and is logically equivalent to !a || b.

https://en.m.wikipedia.org/wiki/Material_conditional

@dennisdoomen
Copy link
Member

This is where my lack of CS degree shows ;-)

@jnyrup
Copy link
Member

jnyrup commented Apr 12, 2022

The api and implementation are both straight forward, but my immediate gut feeling is that it will only be needed/used by a few.

If you haven't done it already, you could create Imply as an extension method on BooleanAssertions in your code base.

@94sedighi
Copy link
Contributor

@jnyrup i will take a look on it and try to make the extension.

@jnyrup
Copy link
Member

jnyrup commented Nov 28, 2022

@94sedighi Please wait until the API is approved before opening a PR.

@jnyrup jnyrup mentioned this issue Dec 5, 2022
8 tasks
@dennisdoomen
Copy link
Member

Albeit a bit of a niche request, I have no issue with this proposal. It doesn't add complexity, so it should be easy to support.

@jnyrup
Copy link
Member

jnyrup commented Dec 27, 2022

Alright, then I'll approve the API addition too.

public class BooleanAssertions<TAssertions>
{
    public AndConstraint<TAssertions> Imply(bool expected, string because = "", params object[] becauseArgs)
}

@jnyrup jnyrup added api-approved API was approved, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Dec 27, 2022
@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 28, 2022

Is there a way to extract the expectation variable name?

This seems a little odd in the failure message:
bla.Should().Imply(blubb) would result in Expected "bla" to imply "[variable name inside the method]", but it did not.

But IMO the failure message should contain both, like: Expected "bla" to imply "blubb", but it did not.

@jnyrup
Copy link
Member

jnyrup commented Dec 28, 2022

Is there a way to extract the expectation variable name?

Not currently.
We would like to look into using [CallerArgumentExpression] for this purpose. This is listed in #1677.

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 28, 2022

Ahhm.. @94sedighi I've missed the comment where you "saved" this issue.

Sorry :)

@94sedighi
Copy link
Contributor

Ahhm.. @94sedighi I've missed the comment where you "saved" this issue.

Sorry :)

Allright 😉

@dennisdoomen
Copy link
Member

We would like to look into using [CallerArgumentExpression] for this purpose.

I think we can already use that in this API

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 28, 2022

How would you add this argument?

public AndConstraint<TAssertions> Imply(bool implicator,
        string because = "",
        [CallerArgumentExpression("implicator")] string implicatorMessage = "",
        params object[] becauseArgs)

or

public AndConstraint<TAssertions> Imply(bool implicator,
        [CallerArgumentExpression("implicator")] string implicatorMessage = "",
        string because = "",
        params object[] becauseArgs)

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 28, 2022

Hmm.. what I can see by now: we have to go with approach 2, because if you pass becauseArgs the first one is considered as custom CallerArgumentExpression

@jnyrup
Copy link
Member

jnyrup commented Dec 29, 2022

Using approach 2 is also problematic

Writing this idiomatic FA code

subject.Should().Imply(implicator, "because we want to test the {0}", "failure");

fails with this unexpected message

Expected subject (True) to imply "because we want to test the {0}" (False) because failure, but it did not.

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 30, 2022

Ok I see. Unfortunately this param has to be optional.

What we can do? Can we force users to use this parameter?

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 30, 2022

Or maybe this only a documentation issue?

@jnyrup
Copy link
Member

jnyrup commented Dec 30, 2022

I think we should stick with the approved API, i.e. not using [CallerArgumentExpression].
Other APIs that might also benefit from having the name of the expectation, like ReferenceTypeAssertions.BeSameAs, would face the same problem.

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Dec 30, 2022

ok.. I am fine with that.

@dennisdoomen
Copy link
Member

Ok I see. Unfortunately this param has to be optional.

Ah, I see now. So yes, I agree with @jnyrup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved, it can be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants