Split assertions to traits

[rquadling]

The docblocks for the traits should not contain the all* or nullOr* methods as the traits do not handle them.

[rquadling]

This will also be a major version upgrade due to a drop in support of PHP 5.3.

[b1rdex]

The docblocks for the traits should not contain the all or nullOr methods as the traits do not handle them.

Yes, but that's poor maintainability. I defined abstract __callStatic method in traits and I hold to leave magic docblocks with code.

[rquadling]

If the trait docblock is going to retain the __callStatic() handled methods, then the document generator in /bin needs to be updated to automatically create them as that is how they are maintained at the moment.

I would still say that they shouldn't be present at all, but if they are, they need to be auto generated to reduce the maintenance overhead.

[b1rdex]

Oh, I didn't realize they are generated by script. Ok, it would be simplier to bring them back to Assertion class.

[b1rdex]

@rquadling moved them back and removed abstract method.

[rquadling]

Nice.

Can the const's be moved too? I'd go with a renumbering also so use a value that is composed to represent the some logical grouping.

1xxxx = Type related (11xx Array, 12xx Boolean, etc.) 2xxxx = Class related (property, method, etc). etc.

[b1rdex]

Traits can't have constants.

On Apr 18, 2017 21:33, "Richard Quadling" notifications@github.com wrote:

Nice.

Can the const's be moved too? I'd go with a renumbering also so use a value that is composed to represent the some logical grouping.

1xxxx = Type related (11xx Array, 12xx Boolean, etc.) 2xxxx = Class related (property, method, etc). etc.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/beberlei/assert/pull/223#issuecomment-294796695, or mute the thread https://github.com/notifications/unsubscribe-auth/AATGF3Aj4wjPu5sk8k2T9kS8_nYU2UoSks5rxJ-fgaJpZM4M-_cj .

[rquadling]

Ah. Yes. Odd that PHP can have static members, and properties, but not constants. We should have all the things in all the places.

So. Things look good. Dropping support for PHP < 5.4 is fine by me. I've no intention of really supporting something that EOL'd just for the sake of it.

[rquadling]

I think I'd like the traits in their own sub-directory of Assert.

That way the traits are a subordinate of the Assertion class.

[b1rdex]

Ok I moved traits to namespace Assert\Assertion. The only thing about consts I have on mind — they can be namespace constants and placed along with trait using it. Example:

namespace Assert\Assertion;

const INVALID_SUBCLASS_OF = 29;
const INVALID_CLASS = 105;
const INVALID_INTERFACE = 106;
const INTERFACE_NOT_IMPLEMENTED = 202;

trait ClassTrait
{
}

Then we can use them in trait as follows: throw static::createException($value, $message, INVALID_SUBCLASS_OF, …

And in other classes: throw static::createException($value, $message, Assertion\INVALID_SUBCLASS_OF, …

[rquadling]

I like the idea of the traits having their constants locally. Go with the renumbering also. Maybe if you can draw up the renumbering to first. I can review that prior to committing. Good work so far!

[b1rdex]

Moving constants out from Assertion class will break BC and all tests.

[rquadling]

This is going to be a major release as we are dropping support for PHP 5.3.

But namespaced constants vs class constants ... hmmmm.

I'm happy with the BC. An UPGRADE_3.0.md will be needed to tell devs to rename Assertion::CONSTANT to Assertion\CONSTANT.

To maintain BC, pulling the namespaced constants into the Assertion class is also a possibility for 3.0 and then drop them in 4.0. That can be done using a fairly simple update to the doc generator or probably better to rename the doc generator to something more generic (updating the composer.json entry for generating the docs and updating the CONTRIBUTING.md)

[b1rdex]

Had some merge/force push bug, now all is fixed. I didn't renumbered constants, jsut moved them.

[rquadling]

Any chance the scalar assertion could move to its own trait? That way all the assertions are in traits.

MAYBE, a scalar trait uses the boolean, string, number, etc. traits?

I'm really liking the segmentation. Good work!!!

[rquadling]

Renumbering the constants would be a nice touch.

[b1rdex]

OK, I moved scalar out to trait and make it use bool, string and number. Don't know if it's really good…

Also for renumbering: I'm not sure how to do that best, so I leave it for somebody else.

[rquadling]

The unit tests need to have the corrected class constant.

You should be able to use the regular expression Assertion::(?=[A-Z]) to locate all occurrences of the deprecated constants, and replace them with Assertion\.

[rquadling]

Can you also update the UPGRADE_3.0.md doc to use that regex as the one you have will replace all the static calls also.

[rquadling]

Also, take a read of https://thephp.cc/news/2016/02/questioning-phpunit-best-practices regarding the unit tests.

[b1rdex]

Done.

[rquadling]

Care to split the unit tests into 1 file per trait?

[b1rdex]

Done.

[rquadling]

Excellent!!! Final review tomorrow!! Hopefully.

[b1rdex]

Yes, I changed it.

15 дек. 2017 г. 3:30 ДП пользователь "Richard Quadling" < notifications@github.com> написал:

@rquadling commented on this pull request.

In CONTRIBUTING.md https://github.com/beberlei/assert/pull/223#discussion_r157010542:

@@ -4,6 +4,5 @@ Thanks for contributing to assert! Just follow these single guidelines:

• You must use feature / topic branches to ease the merge of contributions.
• Coding standard compliance must be ensured before committing or opening pull requests by running composer assert:cs-fix or composer assert:cs-lint in the root directory of this repository. -- After adding new assertions regenerate the README.md and the docblocks by running composer assert:generate-docs on the command line. +- After adding new assertions regenerate the README.md and the docblocks by running composer assert:generate on the command line.

It is composer assert:generate-docs, unless you've changed it in this branch.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/beberlei/assert/pull/223#pullrequestreview-83583442, or mute the thread https://github.com/notifications/unsubscribe-auth/AATGF1F_7e96PozXGpFZY_nOlV4vpgtaks5tAVsXgaJpZM4M-_cj .

[b1rdex]

@carusogabriel you broke the tests 🤔 see https://travis-ci.org/beberlei/assert/builds/316258002?utm_source=github_status&utm_medium=notification for details.