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

Use static factory methods instead of functions in the Assert namespace #184

Merged
merged 26 commits into from
Nov 14, 2016

Conversation

tacovandenbroek
Copy link
Contributor

The functions in the Assert namespace can not be autoloaded, nor can they be used with alternative exception classes. This pull request resolves these issues by introducing a static factory class (\Assert\Assert) that provides these functions as static methods. This will also solve #57.

@rquadling
Copy link
Contributor

Aha. You've added a new method Assert\AssertionChain::setAssertionClassName() which is not directly involved in the lazy assertions.

So, just like the constructor and the magic call handler, this needs to be added to the list of ignored methods in \MethodDocGenerator::gatherAssertionChainSwitches()

@rquadling
Copy link
Contributor

When you run the composer assert:generate-docs this should correctly build the documentation with any changes you need.

@tacovandenbroek
Copy link
Contributor Author

Updated the Pull Request to support all PHP versions that are supported by the Assertion library

@rquadling
Copy link
Contributor

Hi.

Looking good.

I would like the testing to be done slightly differently though.

With this PR, the existing namespace'd functions (lazy(), that(), thatAll() and thatNullOr()) whilst deprecated, are still going to be needing support until the next major release and these now exist without tests.

So, maybe, reinstate the tests for the functions but in a separate file that can be deleted as part of the deprecation process in the next major release.

Also, the documentation should document both mechanisms, clearly marking the deprecation of the functions in the next major release.

Also, being picky, no tests exist for \Assert\LazyAssertion::setExceptionClass() or \Assert\AssertionChain::setAssertionClassName.

:-D

@rquadling
Copy link
Contributor

And just to make your life a little harder, you'll need to take into account the new tryAll() functionality for chained assertions.

@tacovandenbroek
Copy link
Contributor Author

Besides my stubbornness regarding array syntax I think all should be well now ;)

@tacovandenbroek
Copy link
Contributor Author

Maybe dropping support for PHP versions that have been eol for over two years is a good idea for the next major release?

@rquadling
Copy link
Contributor

I think that dropping support for PHP 5.3 in the next major release is probably reasonable.

The use of the dev namespace threw me a bit.

@rquadling rquadling merged commit a67d496 into beberlei:master Nov 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants