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 implies as logical assertion #225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roelvanduijnhoven
Copy link

I extracted this logical assertion from our code-base. It helps us in cases where we want to assert transitions between the states something can be in. But it can be used in more situations.

This is en existing logical operator, see: https://en.wikipedia.org/wiki/Material_conditional. Not sure how well known it is, but it makes sense once you start using it.

So: let's say you want to assert the input of method updateState($old, $new):

  • You want it to only allow you to go from PENDING to ACTIVE, you can assert using:

    Assert::implies($old === PENDING, $new === ACTIVE);

    Which in speech would translate to: If the old status is PENDING, the new status must be ACTIVE.

    And you could also write this as follows, but note how that does not communicate the actual intent.

    Assert::true($old !== PENDING || $new === ACTIVE);
  • Or you want to assert that you can only reach TERMINATED either from ACTIVE or PENDING, that would be;

    Assert::implies($new === TERMINATED, in_array($old [ACTIVE, PENDING]));

    Which in speech would translate to: If the new status is TERMINATED the old status must be either ACTIVE or PENDING.

    And you could also write this as follows:

    Assert::true($new !== TERMINATED || !in_array($old [ACTIVE, PENDING]));

Would love to hear if anybody thinks this is something you consider adding to this repo.

@roelvanduijnhoven
Copy link
Author

Tests fail. I'll wait to hear feedback on this PR before I fix those.

@Nyholm
Copy link
Collaborator

Nyholm commented Jan 18, 2021

Could you please rebase this PR to make sure the tests are green?

@roelvanduijnhoven roelvanduijnhoven force-pushed the feature/implies branch 9 times, most recently from 7a4dfaa to 2314a37 Compare January 25, 2021 16:22
This assertion can be used to easily assert stuff like:

> Assert::implies($hasDog, $leashAttached);

Which will assert that $leashAttached is true, if $hasDog is true.

This is a shorthand for `Assert::true(!$hasDog || $leashAttached)`.
@roelvanduijnhoven
Copy link
Author

Allright @Nyholm I rebased and squashed my commits. Tests are now green. Only the Code Coverage fails, but it looks like this is not related to what I have done.

The requested page was not found, or you have no access to view it.

@zerkms
Copy link
Contributor

zerkms commented Jan 26, 2021

to @ all

what do you think about changing the signature to

    /**
     * @psalm-pure
     *
     * @param bool $p
     * @param bool $q
     * @param string $message
     *
     * @throws InvalidArgumentException
     */
    public static function implies($p, $q, $message = '')

I personally hate type coercion and think that negation of mixed is always GIGO.

Thoughts?

@roelvanduijnhoven
Copy link
Author

@zerkms I just copied what I already found out in the existing code base.

However I think this is done to support stuff like:

Assert::implies(count($people), count($seats))

I.e.: if there are people, there should be seats.

Anyhow: let me know if I do need to change to booleans.

@zerkms
Copy link
Contributor

zerkms commented Jan 27, 2021

Assert::implies(count($people) > 0, count($seats) > 0)

^ this code expresses the domain requirement explicitly: if there are any people - then there should be some seats. To me if (0) (in languages with boolean type available) makes very little sense.

in the existing code base

In other places some extra assertions should be applied then as well.

let me know if I do need to change to booleans.

I'm just another contributor here and don't make decisions on my own :-)

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

Successfully merging this pull request may close these issues.

None yet

3 participants