beberlei / assert

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

Restore factory functions #233

Closed shadowhand closed 6 years ago

shadowhand commented 7 years ago

The factory functions removed in #184 were helpful in that they provided the same fluent interface with less code. I would like to propose that the following functions be restored as aliases (or perhaps a facade) to the static Assert:: methods:

use function Assert\that;
use function Assert\lazy;

I see no reason to have thatAll or thatNullOr be included, since they are just shortcuts to existing functionality.

texdc commented 7 years ago

I agree. I don't have support for lazy, yet. But, I've put together a few extra assertions and (not to step on assert's toes) a verify factory function. See texdc/guard.

rquadling commented 7 years ago

Hmm, I thought all we did was namespace them. They should operate in exactly the same way.

The advantage is that the static methods are now autoloaded and so don't need any additional logic.

Composer doesn't allow a library to declare a file to be automatically included. If it did, then the aliases could be defined in that class and no need to manually include them.

I'm not convinced that an autoloaded static method is any worse than a manually "included" namespaced function.

Happy to be shown why it is better.

As for the fluent interface, this should have been preserved. Can you show where this is wrong?

shadowhand commented 7 years ago

Composer doesn't allow a library to declare a file to be automatically included. If it did, then the aliases could be defined in that class and no need to manually include them.

What about the files portion of autoload? https://getcomposer.org/doc/04-schema.md#files

rquadling commented 7 years ago

The user still needs to add the entry. If you add an entry there for this package, it doesn't travel to the user's setup. Or. At least used to. Looking for the documentation on that.

rquadling commented 7 years ago

Which I can't find. Hmm. Will need to test this.

shadowhand commented 7 years ago

I'm quite positive that files entries are autoloaded the same as classes, aka automatically included. equip/assist does it.

rquadling commented 7 years ago

Yep! So of course it does! Total brainfart!! Polyfills use this technique all the time! Make a PR.

rquadling commented 7 years ago

Hmmm. https://github.com/beberlei/assert/blob/master/composer.json#L38 Seems we already support this. So is the issue solvable just by removing the @deprecated notices?

rquadling commented 7 years ago

How do you use the functional constructors with a subclass of Assertion?

shadowhand commented 6 years ago

@rquadling you wouldn't use the functional constructors with a subclass. The correct solution would be to subclass Assertion and then write equivalent functional constructors.

rquadling commented 6 years ago

I'm not sure what we are solving by re-introducing the functional constructors over static method constructors?

shadowhand commented 6 years ago

Personal preference. Less code to type:

Assert::that() // vs
that()
rquadling commented 6 years ago

I'd be happy with namespaced Assert\that() but polluting the global namespace always feels wrong. But if we have Assert\that(), then there's not much of a leap to Assert::that().

shadowhand commented 6 years ago

@rquadling the PR attached to this issue uses a namespaced function, usage as:

use function Assert\that();
rquadling commented 6 years ago

If you can add some verbiage to the README.md regarding what to do if you've subclassed assertion and want to use that(), etc., then I'll add this. As this is a removal of deprecation, it is a patch level change in my mind.

rquadling commented 6 years ago

Merging today.

shadowhand commented 6 years ago

@rquadling was going to follow up on this today (busy couple of weeks) and saw that you already merged it. Do you want me to open another PR to add notes about subclassing with functional constructors?

rquadling commented 6 years ago

A new PR would be easier.