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: https://github.com/brick/date-time/pull/53#discussion_r892892182

It can also ignores rules and sniffs on specific files which is something I used to provide a solution to your request here: https://github.com/brick/date-time/pull/53#discussion_r892892182

I've introduced the same sniff doctrine is using to provide a solution to your request here: https://github.com/brick/date-time/pull/53#discussion_r895071370

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 :sweat_smile:

[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: https://github.com/composer/composer/issues/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]

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

Here's a more in-depth discussion about the problem: https://github.com/composer/composer/issues/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 :slightly_smiling_face: