Coding standard (via PHP CS Fixer)

[alexeyshockov]

@BenMorel, what do you think about it? We can also include PHP CS Fixer as a checker, to break the build if the code doesn't follow chosen rules.

[alexeyshockov]

I'm not sure if you have a global standard for brick org, maybe it's good time to define this check for one repo and rollout later to other.

[BenMorel]

Hi, a couple questions:

• Any reason why you use php-cs-fixer vs phpcs? If you happen to have used both, can you please share your experience? I've also read about ECS that can combine both, do you have an opinion about that?
• How do you plan to integrate this with CI? I guess the fixer mode does not make sense, so would CI run with --dry-run and fail if any violation is reported?

[BenMorel]

Another one: I've run php-cs-fixer with your config, and I can see that amongst the stuff that was "fixed", a lot goes against what I would choose as a coding standard for this project.

Some examples:

• inconditionally using self instead of the class name
• un-capitalize the first letter of inline docs
• yoda conditions

What would be the best approach here? Start with the Symfony rules and override some of them? Use another base coding standard? Start a list of fixes from scratch?

[bdsl]

I think it would make sense to try and find a coding standard that aligns well with how @BenMorel writes PHP or wants to write it, and then merge in something to enforce that. That PR would need to do any remaining fixes at the same time, since as far as I know there isn't any base-lining facility in a code style checker. It has been requested for PHP_CodeSniffer: https://github.com/squizlabs/PHP_CodeSniffer/issues/2543

You could start with a config like the following, which the code here follows almost 100%. There's just one violation in src and five in tests.

<?php

return \PhpCsFixer\Config::create()
    ->setRiskyAllowed(true)
    ->setRules([
        '@Symfony' => true,
        '@Symfony:risky' => true,

        'binary_operator_spaces' => false,
        'concat_space' => false,
        'increment_style' => false,
        'native_function_invocation' => false,
        'no_superfluous_phpdoc_tags' => false,
        'phpdoc_align' => false,
        'phpdoc_annotation_without_dot' => false,
        'phpdoc_inline_tag' => false,
        'phpdoc_summary' => false,
        'phpdoc_to_comment' => false,
        'return_type_declaration' => false,
        'self_accessor' => false,
        'single_line_throw' => false,
        'trailing_comma_in_multiline_array' => false,
        'unary_operator_spaces' => false,
        'yoda_style' => false,
    ])
    ->setFinder(PhpCsFixer\Finder::create()->files()->in([__DIR__ . '/src', __DIR__ . '/tests'])->name('*.php'));

Then subsequent PRs could add more rules a few at a time either by putting back some of those from the Symfony set or by adding other rules, e.g. perhaps to ban yoda conditions and assignment in conditionals if that's your style.

Happy to PR this including the CI implementation and try to make sure it's easy to understand for other contributors.

Ideally I think an automatic fixer might be set up to see style problems and fix them itself instead of asking humans to do the fixes.

[bdsl]

You can also ban yoda-style if you want, using 'yoda_style' => ['equal' => false, 'identical' => false, 'less_and_greater' => false],

[vv12131415]

@bdsl there is baselining functionally for php codeSniffer. See https://github.com/DaveLiddament/sarb

[bdsl]

@vladyslavstartsev Ah nice. I actually heard Dave Liddamen give a talk about static analysis and Sarb at PHP London before it was released. Didn't realize it supported PHP CodeSniffer. That could be a good option then.

[bdsl]

Any reason why you use php-cs-fixer vs phpcs

I've used both, although recently only php-cs-fixer. I think either one is probably fine. I think the big differentiator is that php-cs-fixer is all about the automatic fixing and generally only reports issues that it can automatically fix. So people just need to run the fix command before submitting their PRs if it fails. PHP CodeSniffer more often reports problems that it doesn't have an automatic fix for, like lines being too long.

[bdsl]

Also with PHP CodeSniffer you can use comments to turn it off if there are sections of code where you think the usual rules shouldn't apply. PHP-CS-Fixer does not allow that.

[c33s]

if i hadn't setup php-cs-fixer and php-code-sniffer in my projects already i would give https://github.com/symplify/easy-coding-standard a try as it combines php-cs-fixer and php-code-sniffer and allows to exclude files.

so i would go for ecs & phpstan

https://tomasvotruba.com/blog/2017/05/03/combine-power-of-php-code-sniffer-and-php-cs-fixer-in-3-lines/

[TomasVotruba]

@c33s Hi, just came across this randomly. Let me know if you need any help with setting up ECS. I can send PR :)

[BenMorel]

@TomasVotruba You're welcome to send a PR! 🙂 I actually tried ECS the other day on another project, but didn't go very far. I'd be interested to see a setup that gets as close as possible to the current coding style of this project.

Or, if there's a de-facto standard that's close enough to the current coding style, that this project could benefit from at the cost of minor coding style changes, I'm all ears as well (it may be better than having a complex set of rules).

[TomasVotruba]

@BenMorel I'll try to make first steps more pleasant :)

There you go: https://github.com/brick/math/pull/54