Use brick's organization coding standards

[tigitz]

This aim to use the organization's coding standards.

Needs brick/coding-standards#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]

Should this still be in tools/ecs? How will we handle this if we include Psalm in the coding-standards repo?

[tigitz]

Yes it should stay isolated because brick/coding-standards is actually a composer package too that just wrap ECS and some additional packages for specific sniffers.

However you raise a good point about psalm. This meta package cannot group multiple tools into one package as it would fall into the dependency problem we tried to avoid in the first place.

The best option IMO would be to let each brick libraries require the tools independently and only use the coding-standards repo to share tools configuration files and github workflows.

This would bring some flexibility.

Downside is, every time there's a new tool version, each brick libraries have to be updated one by one. Could be automated at some point though.

AFAIU this what doctrine is doing.

[BenMorel]

Will this workflow run both ECS & Psalm from the coding standards repo, or do you want to separate them into different workflows? In the latter case, I'd prefer to keep the name coding-style if we're going to run ECS only.

I basically consider the coding standards repo as containing:

• coding-style: ECS
• static-analysis: Psalm

WDYT?

[tigitz]

Yeah agreed, let's add psalm workflow to this repository and properly split things like you mentioned.

[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 :)