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 a not switch on Assert class #265

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

Conversation

Taluu
Copy link

@Taluu Taluu commented Dec 9, 2018

Add a not helper method on Assertion to "negate" an assertion. As all does, it just adds a not in front of the method name, before the add is added.

@Taluu Taluu force-pushed the add-not-switch-assertion-chain branch from 9931983 to c17785c Compare December 9, 2018 21:11
@Taluu
Copy link
Author

Taluu commented Dec 9, 2018

For some reasons, on the travis, I have some failure, and I don't really understand why as it's nothing I touched ?

Especially as the PR I opened before (#264) was working correctly...

@Taluu Taluu force-pushed the add-not-switch-assertion-chain branch from c17785c to eadce1b Compare December 11, 2018 22:34
@rquadling
Copy link
Contributor

I took your code and updated the documentation.

@rquadling
Copy link
Contributor

And then found issues! Sigh. It's late in the year.

I've reverted the change to master for this code and will leave your pull request open. If you can update it with master and then work on the following:

  1. not should operate and be documented just like nullOr and all - \MethodDocGenerator::generateAssertionDocs() deals with this. I'd add something like : $this->generateMethodDocs($this->gatherAssertions(), ' * @method static bool %s(%s) %s is not true.', $skipParameterTest, 'not'). May need to adjust the grammar on some of the methods for this to be valid in all instances.
  2. Introduce not handling in \Assert\Assertion::__callStatic(). The issue I had was that the message to be displayed needs to be the invert of the message for the assertion being negated. You need to catch the exception from making the call to the non not form (that will return true), and then throwing an exception with the appropriate message (the hard bit here).
  3. Negated unit tests.
  4. Resolve/handle the double negatives:
 * @method static bool notNotBlank(mixed $value, string|callable $message = null, string $propertyPath = null) Assert that value is not blank is not true.
 * @method static bool notNotContains(mixed $string, string $needle, string|callable $message = null, string $propertyPath = null, string $encoding = 'utf8') Assert that string does not contains a sequence of chars is not true.
 * @method static bool notNotEmpty(mixed $value, string|callable $message = null, string $propertyPath = null) Assert that value is not empty is not true.
 * @method static bool notNotEmptyKey(mixed $value, string|int $key, string|callable $message = null, string $propertyPath = null) Assert that key exists in an array/array-accessible object and its value is not empty is not true.
 * @method static bool notNotEq(mixed $value1, mixed $value2, string|callable $message = null, string $propertyPath = null) Assert that two values are not equal (using == ) is not true.
 * @method static bool notNotInArray(mixed $value, array $choices, string|callable $message = null, string $propertyPath = null) Assert that value is not in array of choices is not true.
 * @method static bool notNotIsInstanceOf(mixed $value, string $className, string|callable $message = null, string $propertyPath = null) Assert that value is not instance of given class-name is not true.
 * @method static bool notNotNull(mixed $value, string|callable $message = null, string $propertyPath = null) Assert that value is not null is not true.
 * @method static bool notNotRegex(mixed $value, string $pattern, string|callable $message = null, string $propertyPath = null) Assert that value does not match a regex is not true.
 * @method static bool notNotSame(mixed $value1, mixed $value2, string|callable $message = null, string $propertyPath = null) Assert that two values are not the same (using === ) is not true.

I like the idea of having an assertion and just putting not in front of it and it works. Ideally without an additional method. I doubt it will be possible to do this easily, but if you can come up with a creative way to get the messages right, then I think this will be a good feature.

@Taluu
Copy link
Author

Taluu commented Dec 24, 2018

I also thought about preventing the notNot calls. Either by preventing a not() call or inverting it (notNotBlank becomes blank). I'd rather add the first check though

I'll try to come up with something after the holidays :)

@Taluu
Copy link
Author

Taluu commented Dec 24, 2018

Note that I also thought about just catching the exception and return true, but as you said, the hard bit is creating the alternate message...

@rquadling
Copy link
Contributor

For the double nots, I think just removing the existing notXxxx method in favour of the automatic notXxxx method (if that makes sense). Essentially, anyone writing a new assertion will automatically have the invert of their assertion available. No need to manually create the not variant.

Thanks for the work so far. I've just released v3.2.0. Have a good XMas.

@Taluu
Copy link
Author

Taluu commented Dec 27, 2018

After a "quick" look, I don't think it really is easily manageable to add a not switch on the __callStatic, for the following reasons...

  1. As you already mentionned it, negating a message is not easy actually.
  2. Related to 1., but if we just add a try / catch, we have no way of fetching the exception message from the method name, as the parameters can vary actually.

So IMO, a first simple step is as I made it, and assume that a notXXX method exists for now (otherwise, the __callStatic will throw a BadMethodException and that's it for now). Especially as combinations with nullOr or all methods could be interesting (and more or less supported moreover).

while I do agree that having to manually write the notXXX assertions is a pain, especially as some methods does not make any senses to have a notXXX (e.g minCount, use maxCount - 1 instead), I think it's a minor and manageable pain for now... Especially as the difficulties mentionned above are a real pain not easily manageable IMO.

@Taluu Taluu force-pushed the add-not-switch-assertion-chain branch from eadce1b to 01ed0bc Compare February 5, 2020 09:51
@Taluu
Copy link
Author

Taluu commented Feb 5, 2020

Rebased onto upstream/master.

Ping @rquadling

(Still looking for suggestions on how to handle things properly)

@Taluu
Copy link
Author

Taluu commented Feb 5, 2020

(Failures unrelated)

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.

2 participants