brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

Use brick's organization coding standards #62

Open tigitz opened 2 years ago

tigitz commented 2 years ago

This aim to use the organization's coding standards.

Needs https://github.com/brick/coding-standards/pull/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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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

image

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 :+1: 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 commented 2 years ago

@BenMorel friendly ping 😉

BenMorel commented 2 years ago

@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 commented 2 years ago

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

tigitz commented 1 year ago

@BenMorel friendly ping :)