Split assertions to traits #223

b1rdex · 2017-04-17T08:58:45Z

No description provided.

rquadling · 2017-04-17T12:29:25Z

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

rquadling · 2017-04-17T12:33:13Z

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

b1rdex · 2017-04-18T02:56:39Z

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 · 2017-04-18T09:18:49Z

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 · 2017-04-18T09:55:53Z

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

b1rdex · 2017-04-18T10:03:37Z

@rquadling moved them back and removed abstract method.

rquadling · 2017-04-18T11:33:50Z

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 · 2017-04-18T12:33:21Z

Traits can't have constants.

…

On Apr 18, 2017 21:33, "Richard Quadling" ***@***.***> 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 <#223 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATGF3Aj4wjPu5sk8k2T9kS8_nYU2UoSks5rxJ-fgaJpZM4M-_cj> .

rquadling · 2017-04-18T13:04:42Z

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 · 2017-04-18T13:09:51Z

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 · 2017-04-19T01:29:22Z

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 · 2017-04-19T09:41:59Z

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 · 2017-04-20T02:02:35Z

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

rquadling · 2017-04-20T09:25:01Z

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)

bump required php version remove php 5.3 travis add abstract __callStatic move docblocks back move traits to namespace wip const links generator add upgrade notes split assertions to traits bump required php version add abstract __callStatic remove php 5.3 travis move docblocks back move traits to namespace

b1rdex · 2017-04-21T03:29:13Z

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

rquadling · 2017-04-21T09:52:13Z

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 · 2017-04-21T09:53:01Z

Renumbering the constants would be a nice touch.

b1rdex · 2017-04-26T08:31:02Z

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 · 2017-04-26T11:40:15Z

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 · 2017-04-26T11:41:02Z

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 · 2017-04-26T11:41:23Z

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

b1rdex · 2017-04-27T02:50:47Z

Done.

rquadling · 2017-04-27T12:08:45Z

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

b1rdex · 2017-05-02T02:07:50Z

Done.

rquadling · 2017-05-02T17:30:27Z

Excellent!!! Final review tomorrow!! Hopefully.

carusogabriel · 2017-12-14T04:42:43Z

tests/Assert/Tests/Assertion/ArrayTraitTest.php

+/**
+ * @covers \Assert\Assertion\ArrayTrait
+ */
+class ArrayTraitTest extends \PHPUnit_Framework_TestCase


Please use PHPUnit\Framework\TestCase instead of PHPUnit_Framework_TestCase as #238

@carusogabriel I have no time for this. You are free to open PR against my fork.

Gonna do it so 👍

Phpunit and rebase

b1rdex · 2017-12-14T23:41:43Z

Yes, I changed it. 15 дек. 2017 г. 3:30 ДП пользователь "Richard Quadling" < notifications@github.com> написал: *@rquadling* commented on this pull request. ------------------------------ In CONTRIBUTING.md <#223 (comment)>:

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

- You must use [feature / topic branches](https://git-scm.com/book/en/v2/Git-Branching-Branching-Workflows) 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](README.md) and the docblocks by running `composer assert:generate-docs` on the command line. +- After adding new assertions regenerate the [README.md](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 <#223 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATGF1F_7e96PozXGpFZY_nOlV4vpgtaks5tAVsXgaJpZM4M-_cj> .

b1rdex · 2017-12-15T02:08:40Z

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

carusogabriel · 2017-12-15T02:32:28Z

tests/Assert/Tests/Assertion/CallableTraitTest.php

+/**
+ * @covers \Assert\Assertion\CallableTrait
+ */
+class CallableTraitTest extends TestCase


@b1rdex Forget to use PHPUnit\Framework\TestCase here. I'm sorry

carusogabriel · 2017-12-15T02:33:56Z

tests/Assert/Tests/Assertion/ClassTraitTest.php

+/**
+ * @covers \Assert\Assertion\ClassTrait
+ */
+class ClassTraitTest extends TestCase


@b1rdex Forget to use PHPUnit\Framework\TestCase here. I'm sorry

carusogabriel · 2017-12-15T02:34:10Z

tests/Assert/Tests/Assertion/CompareTraitTest.php

+/**
+ * @covers \Assert\Assertion\CompareTrait
+ */
+class CompareTraitTest extends TestCase


@b1rdex Forget to use PHPUnit\Framework\TestCase here. I'm sorry

rquadling added this to the 3.0.0 milestone Apr 17, 2017

b1rdex force-pushed the master branch from 30f20ea to 290971f Compare April 21, 2017 03:12

move constants again…

253a4ef

fix

36bc5fe

move out scalar()

fb5a022

scalar trait

c541bbc

b1rdex added 2 commits April 27, 2017 12:01

marks constants deprecated and point to correct ones

3257678

replace constants in tests

fba210a

split tests

1611154

Merge branch 'master' into master

5b5d125

carusogabriel suggested changes Dec 14, 2017

View reviewed changes

carusogabriel and others added 3 commits December 14, 2017 03:05

Rebase

1f060dc

Use PHPUnit\Framework\TestCase instead of PHPUnit_Framework_TestCase

01bc95a

Merge pull request #1 from carusogabriel/phpunit-and-rebase

0a122b5

Phpunit and rebase

carusogabriel reviewed Dec 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split assertions to traits #223

Split assertions to traits #223

b1rdex commented Apr 17, 2017

rquadling commented Apr 17, 2017

rquadling commented Apr 17, 2017

b1rdex commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 18, 2017

b1rdex commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 18, 2017 via email

rquadling commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 19, 2017

rquadling commented Apr 19, 2017

b1rdex commented Apr 20, 2017

rquadling commented Apr 20, 2017

b1rdex commented Apr 21, 2017

rquadling commented Apr 21, 2017

rquadling commented Apr 21, 2017

b1rdex commented Apr 26, 2017

rquadling commented Apr 26, 2017

rquadling commented Apr 26, 2017

rquadling commented Apr 26, 2017

b1rdex commented Apr 27, 2017

rquadling commented Apr 27, 2017

b1rdex commented May 2, 2017

rquadling commented May 2, 2017

carusogabriel Dec 14, 2017

b1rdex Dec 14, 2017

carusogabriel Dec 14, 2017

b1rdex commented Dec 14, 2017 via email

b1rdex commented Dec 15, 2017

carusogabriel Dec 15, 2017

carusogabriel Dec 15, 2017

carusogabriel Dec 15, 2017

Split assertions to traits #223

Are you sure you want to change the base?

Split assertions to traits #223

Conversation

b1rdex commented Apr 17, 2017

rquadling commented Apr 17, 2017

rquadling commented Apr 17, 2017

b1rdex commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 18, 2017

b1rdex commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 18, 2017 via email

rquadling commented Apr 18, 2017

rquadling commented Apr 18, 2017

b1rdex commented Apr 19, 2017

rquadling commented Apr 19, 2017

b1rdex commented Apr 20, 2017

rquadling commented Apr 20, 2017

b1rdex commented Apr 21, 2017

rquadling commented Apr 21, 2017

rquadling commented Apr 21, 2017

b1rdex commented Apr 26, 2017

rquadling commented Apr 26, 2017

rquadling commented Apr 26, 2017

rquadling commented Apr 26, 2017

b1rdex commented Apr 27, 2017

rquadling commented Apr 27, 2017

b1rdex commented May 2, 2017

rquadling commented May 2, 2017

carusogabriel Dec 14, 2017

Choose a reason for hiding this comment

b1rdex Dec 14, 2017

Choose a reason for hiding this comment

carusogabriel Dec 14, 2017

Choose a reason for hiding this comment

b1rdex commented Dec 14, 2017 via email

b1rdex commented Dec 15, 2017

carusogabriel Dec 15, 2017

Choose a reason for hiding this comment

carusogabriel Dec 15, 2017

Choose a reason for hiding this comment

carusogabriel Dec 15, 2017

Choose a reason for hiding this comment