Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Composite ruleset.xml #1328

Closed
ikappas opened this issue Feb 1, 2017 · 11 comments
Closed

Composite ruleset.xml #1328

ikappas opened this issue Feb 1, 2017 · 11 comments

Comments

@ikappas
Copy link

ikappas commented Feb 1, 2017

Question/Feature Request

Based on our reference setup, how can we use the stack ruleset.xml to perform a single phpcs operation that covers all areas (stack/plugin/theme)?

Reference Setup

My team works with WordPress using a BedRock like setup like so:

project_root (Stack)
|- config
|- scripts
|- vendor
|- web
|  |- app
|  |  |- plugins
|  |  |  |- the_plugin (Plugin)
|  |  |     |- ruleset.xml
|  |  |     ...
|  |  |- themes
|  |     |- the_theme (Theme)
|  |        |- ruleset.xml
|  |        ... 
|  |- wp
|      ...
|- ruleset.xml

The Stack ruleset.xml is configured like so:

<?xml version="1.0"?>
<ruleset name="WordPress-Stack">
    <description>Coding standards for WordPress stack.</description>
    <exclude-pattern>*/.sass-cache/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/web/app/*</exclude-pattern>
    <exclude-pattern>*/web/wp/*</exclude-pattern>
    <rule ref="WordPress-Core"/>
</ruleset>

The Plugin/Theme ruleset.xml are more strict and configured like so:

<?xml version="1.0"?>
<ruleset name="WordPress-Strict">
    <description>Coding standards for WordPress development.</description>
    <exclude-pattern>*/.sass-cache/*</exclude-pattern>
    <exclude-pattern>*/assets/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <rule ref="WordPress-VIP"/>
</ruleset>

The main areas that we maintain and are interested in are:
/project_root/config, /project_root/scripts based on stack ruleset.xml
/project_root/web/app/plugins/the_plugin using the plugin ruleset.xml
/project_root/web/app/themes/the_theme using the theme ruleset.xml

@aik099
Copy link
Contributor

aik099 commented Feb 1, 2017

Were you able to create composite ruleset? I haven't tried to create ruleset using custom (non-core) standards, but with the help of relative paths it should be possible.

Most likely the exclude-pattern would work from root of checked project and not from ruleset location.

@ikappas
Copy link
Author

ikappas commented Feb 1, 2017

@aik099 No I havent achieved that and that is why I am asking :)

I assume this kind of setup would be fairly common when using some sort of platform (WordPress, Drupal, etc) where we may need to focus on some root files and a few child files grouped as plugins/modules/themes/etc.

I think having a root ruleset that composes the rules from multiple ruleset.xml based on path would be a welcome feature.

As far as I can tell currently the root ruleset.xml can import rules from a child ruleset.xml as seen in https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml like:

 <!--
    Include everything in another ruleset.xml file. This is
    really handy if you want to customise another developer's
    custom standard. They just need to distribute their single
    ruleset file to allow this.
 -->
 <rule ref="/home/username/standards/custom.xml"/>

But that is not our use case. We basically need to use different rulesets against different paths, coordinated by a master/root ruleset.

Using include/exclude patterns, one potential problem is how to distinguish between rules applied to the root and the children paths.

Unless I am missing something! :)

@aik099
Copy link
Contributor

aik099 commented Feb 1, 2017

We basically need to use different rulesets against different paths.

That doesn't work like that. What files to scan you're specifying when running phpcs or phpcbf. This can't be specified statically in your ruleset.xml.

My suggestion would be creating a script that would run phpcs several times for different times and then somehow combine produced error list.

@ikappas
Copy link
Author

ikappas commented Feb 1, 2017

@aik099 Perhaps this can be a new feature?

We have scripts in place to accomplish this for our CI/terminal incorporated in composer.json like so:

    "scripts": {
        "run-phpcs": [
            "@run-phpcs:stack",
            "@run-phpcs:plugin",
            "@run-phpcs:theme"
        ],
        "run-phpcs:stack": [
            "\"vendor/bin/phpcs\" --standard=ruleset.xml --extensions=php -p -n -s --colors ."
        ],
        "run-phpcs:plugin": [
            "\"vendor/bin/phpcs\" --standard=./web/app/plugins/the_plugin/ruleset.xml --extensions=php -p -n -s --colors ./web/app/plugins/the_plugin/"
        ],
        "run-phpcs:theme": [
            "\"vendor/bin/phpcs\" --standard=./web/app/themes/the_theme/ruleset.xml --extensions=php -p -n -s --colors ./web/app/themes/the_theme/"
        ],
        "fix-phpcs": [
            "@fix-phpcs:stack",
            "@fix-phpcs:plugin",
            "@fix-phpcs:theme"
        ],
        "fix-phpcs:stack": [
            "\"vendor/bin/phpcbf\" --standard=ruleset.xml --extensions=php -n ."
        ],
        "fix-phpcs:plugin": [
            "\"vendor/bin/phpcbf\" --standard=./web/app/plugins/the_plugin/ruleset.xml --extensions=php -n ./web/app/plugins/the_plugin/"
        ],
        "fix-phpcs:theme": [
            "\"vendor/bin/phpcbf\" --standard=./web/app/themes/the_theme/ruleset.xml --extensions=php -n ./web/app/themes/the_theme/"
        ]
    },

But this approach runs 3 distinct phpcs operations on:

$ composer run-phpcs

Also, my main interest for this feature is not the scripts and console but linting through a project via an editor (Sublime/VS Code/PhpStorm/etc). All these implementations allow you to configure a single ruleset.xml as the configuration source for phpcs. As such you can only have one configuration active at a time.

This feature would allow to have ruleset.xml in child folders that are coordinated by the root ruleset.xml thus enabling path scoped rules that could even use different standards.

@ikappas
Copy link
Author

ikappas commented Feb 1, 2017

A quick reference root ruleset.xml could look like:

<?xml version="1.0"?>
<ruleset name="WordPress-Stack">
    <description>Coding standards for WordPress stack.</description>
    <exclude-pattern>*/.sass-cache/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/web/app/*</exclude-pattern>
    <exclude-pattern>*/web/wp/*</exclude-pattern>
    <rule ref="WordPress-Core"/>
    <rule ref="./web/app/plugins/the_plugin/ruleset.xml" scope="path"/>
    <rule ref="./web/app/themes/the_theme/ruleset.xml" scope="path"/>
</ruleset>

where the rule <rule ref="./some/path/ruleset.xml" scope="path"/> would tell phpcs to use that ruleset for all linting under ./some/path/

@gmponos
Copy link
Contributor

gmponos commented Feb 1, 2017

I would like also to put my point of view about this issue.

I believe what @ikappas is suggesting will be extra useful.

If you have a composer like this:

{
...
    "scripts": {
        "phpcbf": [
            "php vendor/bin/phpcbf mypath1 --standard=ruleset1.xml",
            "php vendor/bin/phpcbf mypath2 --standard=ruleset2.xml"
        ],
    }
...
}

If the first command finds a fixable file then the second command wont run. Most of the times the developers think that all files are fixed and they push their code only to discover that the CI has failed due to Codesniffer errors.

@aik099
Copy link
Contributor

aik099 commented Feb 1, 2017

@gmponos , that already can be fixed by adding --runtime-set ignore_warnings_on_exit 1 to the phpcs or phpcbf command. Then exit code would always be 0 and won't cause any trouble for Composer.

@ikappas
Copy link
Author

ikappas commented Oct 1, 2017

@aik099 Any progress/update on this feature?

@ikappas
Copy link
Author

ikappas commented Nov 3, 2017

@gsherwood Any consideration on this feature?

@gsherwood
Copy link
Member

Closing as there has been no progress on this and it's no longer on the todo list.

@XedinUnknown
Copy link

Forgive me for this act of necromancy, but I would also really like this feature. In my case, we use templates in a WordPress plugin. The pugin has its own rules, and views (they live in e.g. views/) need to have some of those rules relaxed - like output escaping, because it is performed by the template engine.

Would you recommend to simply use 2 different config files, one for everything but the views, and one for views only?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants