beberlei / assert

Thin assertion library for use in libraries and business-model
Other
2.41k stars 188 forks source link

Add a `not` switch on Assert class #265

Open Taluu opened 5 years ago

Taluu commented 5 years ago

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 commented 5 years ago

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...

rquadling commented 5 years ago

I took your code and updated the documentation.

rquadling commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

Rebased onto upstream/master.

Ping @rquadling

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

Taluu commented 4 years ago

(Failures unrelated)