shvlv
commented
7 months ago Wouldn't it be good to have that discussion or documentation (if the discussion already happened) here in this very PR?
Thanks for pointing this out. The PR was updated after an internal discussion, but it does make sense to document the decision and explain the reasons behind it.
Inpsyde and InpsydeTemplates standards and their relationship
I initially suggested making the InpsydeTemplates coding standard completely decoupled and unrelated (as you can see from the PR description). In this case, even using the standalone repository is possible (but it's not the best idea, as we have a testing framework and primary PHP coding standard documentation here). Also, PHP templates are still regular PHP files requiring other code-quality sniffs. So, we must apply both Inpsyde and InpsydeTemplates sniffs to PHP templates.
It's possible with the decoupled setup like:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
<file>./src/</file>
<file>./tests</file>
<file>./functions.php</file>
<file>./views</file>
<rule ref="Inpsyde">
</rule>
<rule ref="InpsydeTemplates">
<include-pattern>*/views/*</include-pattern>
</rule>
</ruleset>But what if we want turn off some rules (like NoElse) from Inpsyde in the template context? Sure, we can do it in our project-specific standard, but we can't do it in InpsydeTemplates by default.
That's why the "InpsydeTemplates extends the Inpsyde coding standard" option was selected. It provides maximum flexibility (we can conditionally turn off Inpsyde rules, and we can add third-party template-specific sniffs, if any) and not add much verbosity to consuming package config:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
<file>./src/</file>
<file>./tests</file>
<file>./functions.php</file>
<file>./views</file>
<rule ref="Inpsyde">
<exclude-pattern>*/views/*</include-pattern>
</rule>
<rule ref="InpsydeTemplates">
<include-pattern>*/views/*</include-pattern>
</rule>
</ruleset>Regarding "Inpsyde including InpsydeTemplates", it might work as well. Basically, it's the inversion of the initial suggestion:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
<file>./src/</file>
<file>./tests</file>
<file>./functions.php</file>
<file>./views</file>
<rule ref="Inpsyde">
</rule>
<rule ref="InpsydeTemplates">
<exclude-pattern>*/src/*</include-pattern>
<exclude-pattern>*/tests/*</include-pattern>
<exclude-pattern>*/functions.php</include-pattern>
</rule>
</ruleset>But most of the time, it will be more verbose. And still, turning off specific rules from Inpsyde in a template context is not possible.
Last but not least, consider backward compatibility. Changing the Inpsyde coding standard in this way will require updating every project-specific standard.
Based on the listed reasons, the "InpsydeTemplates extends the Inpsyde coding standard" seems the most reasonable.
Would it be possible not to create the standalone standard but add a new sniffs directory?
Let's consider the following structure.
Sniffs
CodeQuality
TemplatesImplementing the selected setup is not possible. Any imported sniffs from PSR, WordPress, etc., in the following ruleset are ignored. Only the CodeQuality and Templates sniffs are imported:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
<file>./src/</file>
<file>./tests</file>
<file>./functions.php</file>
<file>./views</file>
<rule ref="Inpsyde.CodeQuality">
<exclude-pattern>*/views/*</include-pattern>
</rule>
<rule ref="Inpsyde.Templates">
<include-pattern>*/views/*</include-pattern>
</rule>
</ruleset>We have to import the whole Inpsyde standard to import external sniffs. So it equals "Inpsyde including InpsydeTemplates" with discussed already cons (it is impossible to turn off curated Inpsyde rules, it is impossible to include third-party template-specific rules, it breaks backward compatibility and might be more verbose):
<?xml version="1.0" encoding="UTF-8"?>
<ruleset>
<file>./src/</file>
<file>./tests</file>
<file>./functions.php</file>
<file>./views</file>
<rule ref="Inpsyde">
</rule>
<rule ref="Inpsyde.Templates">
<exclude-pattern>*/src/*</include-pattern>
<exclude-pattern>*/tests/*</include-pattern>
<exclude-pattern>*/functions.php</include-pattern>
</rule>
</ruleset>
gmazzap
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Feature.
What is the current behavior? (You can also link to an open issue here) There are no rules targeted to the PHP templates.
What is the new behavior (if this is a feature change)?
PHP templates have specific formatting rules. For some of our projects, we follow Plates-suggested syntax . It includes using alternative syntax for control structures, short echo tags, and avoiding semicolons. Therefore, creating a set of rules targeted to PHP templates makes sense.
The current PR introduces the
InpsydeTemplatesruleset that could be used conditionally (or not used :smile: ) in your projects:For now, only one rule is added to remove trailing semicolons before closing PHP tags. A very nice investigation and usage comparison of this rule can be found here: https://github.com/prettier/plugin-php/issues/609#issuecomment-423863793.
Sure, we can and should discuss the approach widely. Maybe we could decide to maintain such rules in a separate repository. But since we already have a test suite and documentation here, in my opinion, it will be no harm to add custom rules that could be enabled conditionally.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
Other information:
Apart from the feature, there is a small deprecation (since PHPCS 3.8.0) fix here - https://github.com/inpsyde/php-coding-standards/blob/441f7ceca8d0df4ea6bfab9798630cd2031c3fe5/tests/cases/FixturesTest.php#L227-L231. It could be applied separately.