date-time icon indicating copy to clipboard operation
date-time copied to clipboard

Use brick's organization coding standards

Open tigitz opened this issue 3 years ago • 7 comments

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 avatar Jun 21 '22 14:06 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

tigitz avatar Jun 22 '22 19:06 tigitz

@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.

BenMorel avatar Jun 22 '22 21:06 BenMorel

@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 avatar Jul 17 '22 10:07 tigitz

@BenMorel friendly ping 😉

tigitz avatar Jul 21 '22 19:07 tigitz

@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! 👍

BenMorel avatar Jul 21 '22 20:07 BenMorel

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 avatar Aug 21 '22 18:08 tigitz

@BenMorel friendly ping :)

tigitz avatar Nov 30 '22 20:11 tigitz