Use brick's organization coding standards

[tigitz]

This aim to use the organization's coding standards.

Needs brick/coding-standard#2 to be merged to see green CI results.

I've tested it under my own forks as you can see here: https://github.com/tigitz/date-time/actions/runs/2536125238

Library's specific handling of coding standard rules are kept in library's ecs.php file. It uses a $ecsConfig->import(__DIR__ . '/vendor/brick/coding-standards/ecs.php'); to import the shared rules.

I've included a missing DuplicateSpacesSniff that I wasn't able to push before the merging of the previous PR.

[tigitz]

@BenMorel FYI I'm waiting for this to be merged and make sure it works as expected before opening the related PR to other brick projects

[BenMorel]

@tigitz Thanks a lot! I now need to get a better picture of how things will fall into place when both Psalm & ECS will coexist in the coding-standards repo, and how this will translate into other repos such as brick/date-time, both for GitHub Actions & the tools/ directory.

[tigitz]

@BenMorel digging a bit more about the inclusion of psalm as a shared tool for the org, I found that a brick/static-analysis repo might be needed.

Actually, doctrine doesn't share static analysis tools config nor versions https://github.com/doctrine/dbal/blob/3.3.x/composer.json#L45. Their coding standards repo is only meant for coding style tool.

So if we want to do it for static analysis tools too, this would be the ideal setup IMO

Splitting brick/coding-standards in brick/static-analysis and brick/coding-style repos is the result of avoiding dependencies conflicts between the tools.

Another option I thought about was to have a single git repo brick/coding-standards and two static-analysis and coding-style folders in it. But that would bring the overhead of managing a monorepo type of project, running through some hoops so that packagist sees those two folders from a single git repo as actually two distinct packages. Possible but not worth the hassle IMO.

From an usage POV, each brick library contributor would have to follow the same kind of instructions we have right now:

# from project root

# coding style
composer install --working-dir=tools/coding-style
./tools/coding-style/vendor/bin/ecs check --config tools/coding-style/ecs.php

# static analysis
composer install --working-dir=tools/static-analysis
./tools/static-analysis/vendor/bin/psalm -c tools/static-analysis/psalm.xml 

If you're 👍 on the idea, could you please create the appropriate brick/static-analysis repo and ping me when it's done.

I'll start working on it right after.

Thanks!

[tigitz]

@BenMorel friendly ping 😉

[BenMorel]

@tigitz Thank you for pursuing your work on this! I’m sorry I’m on vacation right now, and I’d like to think about this a bit more before multiplying dependencies; I’ll get back to you in ~1 week!

Thanks for your patience! 👍

[tigitz]

Hey @BenMorel, hope your vacations went well 🙂. If you can find some time to help moving this topic forward I'll be available. Thanks in advance.

[tigitz]

@BenMorel friendly ping :)

[BenMorel]

I just rebased your PR, it seems to work fine, apart from the Easy Coding Standard job which is stuck in Expected status, even though it's been removed. I asked Copilot about this, and here is its reply:

The "Easy Coding Standard" workflow is still expected in this PR because GitHub Actions may be referencing workflow configuration files that were present on the base branch (master) at the time the pull request was created or updated. Even if the workflow file for "Easy Coding Standard" has been removed in your PR branch, GitHub evaluates checks based on the workflows defined in the base branch until the removal is merged. As a result, status checks for "Easy Coding Standard" may continue to appear as required or expected until the base branch itself no longer references the workflow.

This sounds weird to me, but it should disappear once we merge.

Please tell me what you think about moving the skips to brick/coding-standard, and we can merge this!

(let's merge with main as a target for now; I will try the coding-standard repo against other libraries before tagging a release.)

[BenMorel]

I made a few adjustments while testing brick/coding-standard on:

• brick/math (branch) (ecs.php)
• brick/money (branch) (ecs.php)
• brick/phonenumber (branch) (ecs.php)
• brick/geo (branch) (ecs.php)

And it works perfectly. I think the defaults are very sensible: as you can see from the ecs.php file in each repo, very few adjustments are needed on a per-project basis (brick/phonenumber does not need any!), which is excellent.

I think I can tag a brick/coding-standard release tomorrow. Release numbers will probably be sequential (just v1, v2), as pretty much any change to this repo can be breaking so semver is, IMO, of little interest.

I will not create a brick/static-analysis repository for now; I don't think it brings much value anymore, as I use either Psalm or PHPStan depending on the project at the moment, and the amount of configuration to share between projects is very small.

@tigitz, I'd like to thank you very much for the work you've done here, and apologize for how long it took me to get back to this.

A shared coding standard for Brick repositories was long overdue, and will help move several repositories closer to their 1.0 release!

[tigitz]

Congrats Ben for this achievement.

I'm glad I could help 🙂

[BenMorel]

brick/coding-standard@v1 has been released.

Thank you, @tigitz!