squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Ignoring some file extensions in a custom standard #659

Closed morozov closed 9 years ago

morozov commented 9 years ago

I want to create a custom ruleset that would check PHP files for compliance with PSR-2, and JS files with Closure Linter. Is there a way to inherit the PSR-2 ruleset but make it not check JS files?

I tried the definition below, but the exclude-pattern directive specified for a ruleset reference is ignored, and JS files are still validated against PSR-2.

<?xml version="1.0"?>
<ruleset name="Test_Ruleset">
    <description>A Test Ruleset.</description>
    <rule ref="PSR2">
        <exclude-pattern>*.js</exclude-pattern>
    </rule>
    <rule ref="Generic.Debug.ClosureLinter"/>
</ruleset>

I know it's possible to run phpcs twice, once for each file type/ruleset like

phpcs --extensions=php --standard=PSR2
phpcs --extensions=js --standard=MyGJSLintStandard

but it's not very convenient.

Is there a way to do this on a ruleset level?

orlangur commented 9 years ago

Looks like exclude-pattern works only in ruleset definiton: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/phpcs.xml#L9

As to me situation in one standard when different files are checked against different set of rules is a bit confusing. Each sniff which supports both JS and PHP could make all code more consistent.

Not sure if it will work in such a way, but: would it be suitable for you to define to separate rulesets, one for .js and another for .php, and then just import both of them in one more ruleset.

morozov commented 9 years ago

As to me situation in one standard when different files are checked against different set of rules is a bit confusing.

It's a perfectly valid situation: we validate .php against PSR-2, and .js against gjslint (both are enterprise standards in their fields). Otherwise, I've never seen a single mention of PSR-2 to be designed for javascript. It seems to work like that in PHP_CodeSniffer probably because the sniffs PSR-2 are based on the PEAR ones, which might be designed for JS as well.

Not sure if it will work in such a way, but: would it be suitable for you to define to separate rulesets, one for .js and another for .php, and then just import both of them in one more ruleset.

Looks like it's exactly what I failed to do. There's a separate ruleset "PSR2" which is imported into "Test_Ruleset", it doesn't work. Am I missing something?

Thank you for suggestion.

aik099 commented 9 years ago

I'm using phpcs for PHP checking and jscs for JS checking.

morozov commented 9 years ago

Using two or more separate tools is fine for personal usage, however for enterprise usage one tool is more convenient, especially given that PHP_CodeSniffer supports gjslint.

gsherwood commented 9 years ago

In the 3.0 branch (to be released as version 3.0 sometime in the future) you can now do what you are after.

This ruleset:

<?xml version="1.0"?>
<ruleset name="Test_Ruleset">
    <description>A Test Ruleset.</description>
    <rule ref="PSR2">
        <exclude-pattern>*.js</exclude-pattern>
    </rule>
    <rule ref="Generic.Debug.ClosureLinter"/>
</ruleset>

Will process like this (I've cut out most of the debug output):

Processing ruleset PHP_CodeSniffer/mystandard.xml
    Processing rule "PSR2"
        => PHP_CodeSniffer/src/Standards/PSR2/ruleset.xml
        * rule is referencing a standard using ruleset path; processing *
        Processing ruleset PHP_CodeSniffer/src/Standards/PSR2/ruleset.xml
            * snip *
        => Ruleset processing complete; included 40 sniffs and excluded 0
        => added rule-specific absolute ignore pattern: *.js
        => added rule-specific absolute ignore pattern for PSR2.Classes.ClassDeclaration: *.js
        => added rule-specific absolute ignore pattern for PSR2.Classes.PropertyDeclaration: *.js
        => added rule-specific absolute ignore pattern for PSR2.ControlStructures.ControlStructureSpacing: *.js
        * snip *
        => added rule-specific absolute ignore pattern for Squiz.ControlStructures.ForEachLoopDeclaration: *.js
        => added rule-specific absolute ignore pattern for Squiz.ControlStructures.ForLoopDeclaration: *.js
        => added rule-specific absolute ignore pattern for Squiz.ControlStructures.LowercaseDeclaration: *.js
        => added rule-specific absolute ignore pattern for Generic.ControlStructures.InlineControlStructure: *.js
    Processing rule "Generic.Debug.ClosureLinter"
        => PHP_CodeSniffer/src/Standards/Generic/Sniffs/Debug/ClosureLinterSniff.php

Which is basically saying that the *.js exclusion is being applied to all sniffs that the PSR2 rule brought in. Then after that, the ClosureLinter tool is being included for all files (although it will only check JS files obviously).

This isn't going to help you now of course, unless you want to checkout the 3.0 version and run that. But it was a good feature request, so I've added it in.

In case you (or anyone else) want to test this on the 3.0 branch, and you don't have any custom sniffs that need to be upgraded to work with 3.0:

git clone -b 3.0 git://github.com/squizlabs/PHP_CodeSniffer.git
cd PHP_CodeSniffer
php bin/phpcs --standard=/path/to/ruleset.xml /path/to/code
morozov commented 9 years ago

@gsherwood thank you, I'll try 3.0. Speaking of which, I couldn't find any documentation (e.g. possible BC breakages, new features, roadmap, etc.) neither on github, nor in blog. Is there something to read about it?

gsherwood commented 9 years ago

Is there something to read about it?

Not yet. I have a draft upgrade guide but I haven't finished all the changes yet. They are extensive, including a complete refactoring of all classes.

So far, the big changes are these:

  - Caching between runs (--cache, --no-cache and "cache" config var) (request #530)
  - Check files in parallel (--parallel=1 for off, --parallel=100 etc) (request #421)
  - Automatic discovery of executable paths (request #571)
    -- Thanks to Sergey Morozov for the patch
  - All sniff errors are now exceptions and caught to stop infinite loops
  - Sniff codes are no longer optional - use them with addError/Warning/Fixable
  - installed_paths config setting can now point directly to a standard instead of the dir above
  - --report=full,summary,info instead of --report=full --report=summary --report=info
  - can now set severity, message type and exclude patterns for entire sniff, category or standard
  - can include a single sniff code, which will exclude all other codes from that sniff
    -- if the sniff is already included by an imported standard, set sniff severity to 0 and include the specific message you want

But the real changes are behind the scenes and a forced upgrade of all custom sniffs to use namespaces. That's what the guide will go through.

If you only use the included sniffs and just customise things using a ruleset, you'll have no problems and wont need to upgrade anything.

morozov commented 9 years ago

Actually, besides slightly customized rules (which I don't worry about at all), I have a custom application around PHP_CodeSniffer which strongly depends on how the latter is built inside. However, I'm totally fine with having to rework what it takes to upgrade to 3.0.

I'm bringing it in order to point to an extensibility problem I faced recently. My tool is developed in order to provide pre-commit hook and pull request validator which display errors only in lines of code changed by commit or pull request respectively. I find it useful for gradual introduction of coding standards to legacy projects.

In fact, what it does is running PHP_CodeSniffer on the changed files and then filtering report by removing errors in the lines not affected by the change.

I want to have phpcbf work in a similar way, but it looks like fixes are done during inspection, so when it comes to filtering the report, it's too late. I was thinking about moving filtering from report (PHP_CodeSniffer_Report::generateFileReport()) to file (PHP_CodeSniffer_File::addError(), etc.), but unlike reports, the file instantiation is not extensible.

Just in case, could this also be (or maybe is it) planned for some future release?

gsherwood commented 9 years ago

Then yes, you're going to have to rewrite that. But hopefully the refactoring I've done makes that easier, or at least cleaner. Part of the motivation was to allow external scripts to have more control over the checking process and configuration files.

A good place to start looking would be here: https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/src/Runner.php#L245

You'll probably use all the normal ruleset stuff, but might want to create a new type of File class and create those instead of the File and DummyFile objects that the main runner does. In fact, you probably don't want most of what the main runner does (caching, forking, interactive etc) so you might just replace the runner and be able to do most of it there.

If you give it a go and need some help, please let me know.