Introduce coding style tool

[tigitz]

Hello @BenMorel !

As discussed together, here's the first step towards contributing more and more.

Styling is always subject to preferences and I'm suggesting the ones I'm using in my own libs here. So feel free to make the final decision on each, whether you want to keep them or not.

I've chosen to allow-risky because IMO tests should cover any unexpected change of logic while using a formatting tool.

To ease the review, here's the list of the most controversial rules applied here:

• ordered_class_elements
• return_type_declaration (no space before)
• phpdoc_order (throws before return)
• native_function_invocation

You can find them here: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/doc/rules/index.rst

I've also split the Psalm and Unit tests into 2 workflows to parallelized the checks and halves execution time.

[BenMorel]

Thanks for the PR, @tigitz! I haven't finished the review yet, but I think you've got 90% of the relevant points already. Enough to keep you busy a bit :)

[tigitz]

@BenMorel Updated !

I took the opportunity to introduced two new rules:

self_accessor
php_unit_strict

[BenMorel]

Just reviewed the self_accessor rule, and I think it actually hurts readability!

class DayOfWeek
{
    public function isEqualTo(self $that): bool {

vs:

class DayOfWeek
{
    public function isEqualTo(DayOfWeek $that): bool {

Mixing both styles is not good though, is there a rule that actually does the opposite?

[tigitz]

@BenMorel Rebased and introduce a different tool (https://github.com/symplify/easy-coding-standard) to be able to use both phpcsfixer rules and phpcs sniffs together as it seems to be required here.

I chose to install it into its own tool directory to avoid dependencies conflicts between the tool and the project dependencies. It's the recommended way to install php-cs-fixer for example.

I've removed the self_accessor rule. I haven't found a rule or sniff that does the opposite.

Thanks to PHPCS sniffs availability it can now put @throws after @return in doc blocks like initially requested here: #53 (comment)

It can also ignores rules and sniffs on specific files which is something I used to provide a solution to your request here: #53 (comment)

I've introduced the same sniff doctrine is using to provide a solution to your request here: #53 (comment)

Let me know if there's any blockers left to tackle. The sooner it's merged the less I'll have to go through the rebasing / conflict management process 😅

[BenMorel]

Thanks, @tigitz! A couple questions:

• what's the advantage of installing tools in a separate directory / composer.json? vs. installing using require-dev? One drawback I can see is that you have to perform 2 composer installs when contributing to the project. And what about Psalm?
• can ecs ( / phpcs) fix automatically the codebase, like php-cs-fixer does?

[tigitz]

@BenMorel

what's the advantage of installing tools in a separate directory / composer.json? vs. installing using require-dev?

Even though the tool is installed in require-dev, its dependencies will have to match the one from the lib and other tools. Which is something that ideally shouldn't happen. A tool and its dependencies shouldn't force an upgrade or a downgrade of the lib dependencies for example.

This seem to be an edge case but actually the problem is quite frequent and this lib is impacted too.

When running composer require --dev symplify/easy-coding-standard

It gives a cryptic error about dependencies conflicts with psalm which shouldn't happen in the first place, both tools shouldn't share dependencies anyway and they should be isolated.

Here's a more in-depth discussion about the problem: composer/composer#9636

One drawback I can see is that you have to perform 2 composer installs when contributing to the project.

A two command line instruction instead of a single one in the contribution part of the readme would be the only drawback I see here. Given the benefit of what it brings on the other hand, I see the whole thing as a net benefit all in all.

And what about Psalm?

If you validate the approach, I can migrate psalm the same way. In this PR's scope or in an other. Up to you.

can ecs ( / phpcs) fix automatically the codebase, like php-cs-fixer does?

Yes it does, I've actually used it to generate theses changes. I'll update or introduce the related command lines to run in the readme.

[BenMorel]

Thanks a lot for your work, @tigitz! The codebase looks in much better shape now.

I've completed the review, let's fix/discuss the last points and it'll be good for merge. If you can complete soon enough, I can stop commits/merges in the meantime to avoid conflicts.

[BenMorel]

AFAICS, this can be simply:

/tools/*
!/tools/ecs/composer.json

[tigitz]

Unfortunately not, I've put a stackoverflow link above to "document" why it's required: https://stackoverflow.com/a/16318111

[BenMorel]

I've checked that link, and I tested both versions, which work equally fine for me.
What issue do you run into when using the two-line version?

[tigitz]

I've tested both version too on my hand and the result is quite different:

I thought it could be a git version problem but upgrading from 2.25 to 2.36 didn't change the behavior.

What makes you think they work equally fine ?

[BenMorel]

Weird, I can't reproduce your issue here. When I remove these 2 lines, git still tracks composer.json and ecs.php.
I guess it doesn't hurt much to leave them, though.

[BenMorel]

Weird that the double-space has not been caught here?

[tigitz]

Well there's no rules to enforce a single whitespace after each array elements right now. Else it would give this

@@ -778,14 +778,14 @@
     public function providerToDateTime(): array
     {
         return [
-            ['2018-10-18T12:34Z',                        '2018-10-18T12:34:00.000000+0000'],
-            ['2018-10-18T12:34:56Z',                     '2018-10-18T12:34:56.000000+0000'],
-            ['2018-10-18T12:34:00.001Z',                 '2018-10-18T12:34:00.001000+0000'],
-            ['2018-10-18T12:34:56.123002Z',              '2018-10-18T12:34:56.123002+0000'],
-            ['2011-07-31T23:59:59+01:00',                '2011-07-31T23:59:59.000000+0100'],
-            ['2011-07-31T23:59:59.02-05:30',             '2011-07-31T23:59:59.020000-0530'],
+            ['2018-10-18T12:34Z', '2018-10-18T12:34:00.000000+0000'],
+            ['2018-10-18T12:34:56Z', '2018-10-18T12:34:56.000000+0000'],
+            ['2018-10-18T12:34:00.001Z', '2018-10-18T12:34:00.001000+0000'],
+            ['2018-10-18T12:34:56.123002Z', '2018-10-18T12:34:56.123002+0000'],
+            ['2011-07-31T23:59:59+01:00', '2011-07-31T23:59:59.000000+0100'],
+            ['2011-07-31T23:59:59.02-05:30', '2011-07-31T23:59:59.020000-0530'],
             ['2011-07-31T23:59:59+01:00[Europe/London]', '2011-07-31T23:59:59.000000+0100'],
-            ['2011-07-31T23:59:59.000123456-07:00',      '2011-07-31T23:59:59.000123-0700'],
+            ['2011-07-31T23:59:59.000123456-07:00', '2011-07-31T23:59:59.000123-0700'],
         ];
     }

Which is something you don't want if I understood correctly

[BenMorel]

I'm interested in having this rule most of the time, but to be able to exclude it when done on purpose to improve readability, like in some of the tests. As you've done for other rules?

[tigitz]

@BenMorel Understood, however in this specific case the DurationTest file has column-aligned formatted arrays which shouldn't be touched so excluding the file is necessary and that will make this particular issue not detected and therefore not fixed.

The only way to exclude the check for specific arrays in the same file would be to use a more fine grained approach using php comments

// phpcs:ignore Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed

But I'm not sure the maintenance burden of applying these comments for every arrays is worth it. WDYT ?

For now I've excluded the whole tests folder from this DuplicateSpacesSniff

[BenMorel]

I guess it's good enough for now, then!

[BenMorel]

By the way, I understand, and can learn to like, the tools/ approach.

Here's a more in-depth discussion about the problem: composer/composer#9636

Thanks for the pointer, I actually tried bamarni/composer-bin-plugin but apart from the symlink in the main vendor/bindirectory (which the current maintainer said he doesn't use himself), I wonder what it brings vs your "manual" approach? Maybe the convenience of composer bin all install/update?

If you validate the approach, I can migrate psalm the same way. In this PR's scope or in an other. Up to you.

Let's do this in a separate PR, along with some instructions in the README on how to setup the whole thing, for new contributors?

[tigitz]

@BenMorel Answered, updated and rebased.

I wonder what it brings vs your "manual" approach? Maybe the convenience of composer bin all install/update?

From my limited knowledge on the tool I would say yes. I don't think it's worth adding another "tool" to manage other tools while composer can already do the trick.

Let's do this in a separate PR, along with some instructions in the README on how to setup the whole thing, for new contributors?

I've added a Contributing section to the README.md

[BenMorel]

@tigitz Thank you, let's figure out the last two points and let's merge!

[BenMorel]

Thanks a lot, @tigitz! If you're interested, we can think about how to deploy the same coding standards to other brick projects, without duplicating the config, composer.json etc. every time. Maybe a separate brick/coding-standard repo that we can use in each library repo?

[tigitz]

@BenMorel happy to contribute 😊

Let's do this yeah. This what doctrine and sylius are doing. I'll take inspiration from them.

Ping me when repo is created, thanks.

[BenMorel]

Here you are: https://github.com/brick/coding-standards 🙂