Flip expected/actual arguments? about assert HOT 15 CLOSED

Thanks @webmozart for opening this issue, it really makes sense to me to revert assertions such as instanceOf. In fact almost all others already comply with the "expected first" concept except it's in the method name:

Assert::$expectedState($value, $message);
Assert::$expectedExression($expectedState, $value, $message)

Totally agreed on the PHPUnit style compliance, and for what it worth it's a big 👍 for me.

from assert.

To reinforce what I already stated: I would only consider doing this change if we have a fully-automated upgrade path.

from assert.

Hm. That is an interesting idea. But it only makes sense in some cases, right?

31 of the assertions looks like this:
isArray($value, $message = '')

26 look like this:
isInstanceOf($value, $class, $message = '')

And then there is a handful that has more parameters. But the general idea is:

• first parameter is $value
• last parameter is $message = ''

What they currently have in common is that all of them starts with the "actual"/$value. Switching almost half of our assertions to be similar to PHPUnit seams strange, because that will not make all our assertion coherent.
What about people that are used to how webmozart/assert does it?

Im not too happy with this change.

from assert.

I prefer the current style because I read it by placing the arguments inside the function name, eg:

Assert::allIsInstanceOf($employees, 'Acme\Employee');
// assert all $employees is instance of 'Acme\Employee'

maybe that's also because I'm not often using phpunit.
but I don't mind that much as I'm not a heavy user of this lib either

from assert.

I think its safe to say that this is not something that will be changed.

from assert.

It depends on what you call coherent/consistent. I don't think PHPUnit assertions are inconsistent. The first argument is always "expected", the second is always "actual". If there is no "expected", then (obviously) the first argument is the "actual".

PHPUnit_Framework_Assert::assertSame($expected, $actual);

// but
PHPUnit_Framework_Assert::assertTrue($actual);

Since the majority of PHP users is (or should be) used to PHPUnit (much more than the users of this library) this should actually be an improvement in DX. I frequently happen to pass arguments in the wrong order - only to realize at some point that my assertion is wrong. That's not what assertions are for.

In any case, if we do this change, it is absolutely mandatory to me that it comes with an upgrade script that allows an automated upgrade of projects using the library.

from assert.

It depends on what you call coherent/consistent

Okey. Fair.
I guess it is a matter of taste/opinion.
And I agree with the fact that one should do as every body else does if there is no obvious win doing it in an other way.

I never had an issue with the order of the arguments. And I am hesitant of bumping to 2.0 for only this feature.

I would like to hear the opinion of other users and contributors. Ping @mathroc @keradus @docteurklein. What do you think? Should we plan this feature for 2.0?

from assert.

also, 👍 for having expected value first.
never have an issue with params order as well, but consistency with PHPUnit would be nice indeed.

Same here, 2.0 for sure, but it could be combined with more BC breakers, but only if there would be any.
I wouldn't postpone this for ages if there would be no other BC breakers.
Like recently there were postponing of minor release for 2 weeks because of some feature still in progress. Don't hesitate to make smaller releases more often.
I'm willing to help managing repo if that would be appreciated.

from assert.

At first I thought I didn't have anything against this proposition, as trying to align with other usages can only be a good thing. However, here are 2 things that just came to my mind:

• is breaking BC more important than following common usage (by common I mean phpunit style)
• if $expected is placed first, you won't be able to have a default value for it.

example with the Assert::throws that just landed: if you don't provide a 2nd arg, it will default to \Exception.
How would you do that with flipped arguments?

now it's only small problems (especially n° 1.): I'm just trying to bring more arguments to the discussion.

cheers!

from assert.

example with the Assert::throws that just landed: if you don't provide a 2nd arg, it will default to \Exception.

For me it's very nice, so one will need to specify what he actually expects, being strict about it.
Now, especially in PHP7 world, one may be confused would default value is \Exception or \Throwable.

from assert.

I tend to agree with @keradus. Explicitly stating

Assert::throws(Exception::class, function () {
    // ...
});

is very explicit, which - in the context of assertions - I think is good.

from assert.

you wont easily have fully automated migration.

1. Project FooAssert use this Assert, then class FooAssert extends Assert.
2. Project App use only FooAssert, where:

$asserter = FooAssert::class;
$asserter::throws(...);

from assert.

What about a fixer with some kind of check, like:

if (method_exists(\Webmozart\Assert:class, $method)
    && ($class === \Webmozart\Assert::class || is_subclass_of($class, \Webmozart\Assert::class))
    && in_array($method, $methodsToFix, true)
) {
    // switch first and second argument
}

?

from assert.

package got, sadly, extra obsolete. changing public API now would be super confusing and brings very little benefit

from assert.

I think this is still a nice to have but as Bernhard already said:

I would only consider doing this change if we have a fully-automated upgrade path.

I hate to get the order of the arguments wrong, but having a BC break without a smooth upgrade path is a no go.

from assert.