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

Selectively disable rules with codingStandardsIgnoreStart and codingStandardsIgnoreLine #604

Closed
westonruter opened this Issue May 28, 2015 · 34 comments

Comments

Projects
None yet
@westonruter
Copy link
Contributor

westonruter commented May 28, 2015

PHPCS supports comment directives like:

// @codingStandardsIgnoreFile
// @codingStandardsIgnoreStart
// @codingStandardsIgnoreLine

However, these are all or nothing. What would be even more useful is if we can selectively ignore certain sniffs or errors within sniffs. For instance:

$q = new WP_Query( array(
    'posts_per_page' => 200, // @codingStandardsIgnoreLine WordPress.VIP.SlowDBQuery.slow_db_query 
) );

or

// @codingStandardsIgnoreStart WordPress.VIP.SlowDBQuery.slow_db_query
$q = new WP_Query( array(
    'posts_per_page' => 200,
) );
// @codingStandardsIgnoreEnd WordPress.VIP.SlowDBQuery.slow_db_query

This draws inspiration from JSCS and its directives like:

//jscs:disable requireCamelCaseOrUpperCaseIdentifiers

We have this in the WordPress-Coding-Standards project in a sniff-specific ad hoc system of end-of-statement comments:

echo $foo; // xss ok

Which suppresses the WordPress.XSS.EscapeOutput sniff. But it would be very useful if this was standardized in PHPCS itself and available to all sniffs

@JDGrimes

This comment has been minimized.

Copy link
Contributor

JDGrimes commented May 28, 2015

👍

@GaryJones

This comment has been minimized.

Copy link
Contributor

GaryJones commented May 29, 2015

What format would the ignoring of multiple disabled rules take? Comma-separation?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

westonruter commented May 29, 2015

Comma-separated rules is what JSCS uses: http://jscs.info/overview.html#disabling-specific-rules

// jscs:disable requireCurlyBraces, requireDotNotation
@jjeaton

This comment has been minimized.

Copy link

jjeaton commented Sep 9, 2015

👍

1 similar comment
@thiemowmde

This comment has been minimized.

Copy link
Contributor

thiemowmde commented Dec 16, 2015

👍

@DanielSiepmann

This comment has been minimized.

Copy link

DanielSiepmann commented Dec 17, 2015

👍

@michaelhogg

This comment has been minimized.

Copy link

michaelhogg commented Dec 23, 2015

👍

@pdelre

This comment has been minimized.

Copy link

pdelre commented Apr 1, 2016

👍

@andrewhowdencom

This comment has been minimized.

Copy link

andrewhowdencom commented May 10, 2016

scss-lint disables lint on a given "scope". See: https://github.com/brigade/scss-lint#disabling-linters-via-source

// scss-lint:disable ExampleSniff

eslint appears to work in a similar way. See: http://eslint.org/docs/user-guide/configuring.html#disabling-rules-with-inline-comments

/* eslint-disable no-alert */

In both cases it's useful to disable select lints for a given file, particularly as you're introducing new requirements over time (such as making code style stricter).

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2016

@Magnum5234

This comment has been minimized.

Copy link

Magnum5234 commented Jul 7, 2017

👍

@gsherwood gsherwood added this to the 3.2.0 milestone Jul 7, 2017

@kenguest

This comment has been minimized.

Copy link
Contributor

kenguest commented Jul 13, 2017

👍

1 similar comment
@mikaelz

This comment has been minimized.

Copy link

mikaelz commented Jul 25, 2017

👍

@widoz widoz referenced this issue Aug 28, 2017

Closed

Improve Wpcs #20

GaryJones added a commit to GaryJones/wp-cli that referenced this issue Oct 5, 2017

CS: Unexclude ValidFunctionName checks
Only one function, and three methods, were found to be violating the preferred WPCS naming conventions, so these lines have been excluded.

While it does mean that the rest of the function signature line is also excluded from other checks, this seems like less of an issue than excluded the whole check from the rest of the project, which may allow other incorrectly named functions and methods to be accidentally introduced.

Once squizlabs/PHP_CodeSniffer#604 is addressed, this ignores should be able to target just the specific check of the function / method name.
@KacerCZ

This comment has been minimized.

Copy link

KacerCZ commented Oct 21, 2017

Slevomat Coding standard has implemented something similar in its sniffs.
https://github.com/slevomat/coding-standard#suppressing-sniffs-locally

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 30, 2017

The prep for this change has been pushed: 5325170

I've added a new shorter comment syntax and put a bit more core support in. The syntax for disabling PHPCS and ignoring lines is now phpcs:disable and phpcs:ignore.

The next step is to get these comments to allow a list of sniff codes after them so they can selectively disable or ignore some errors.

If that's working, a possible extension would be to allow the new phpcs:enable syntax to re-enable selected sniff codes only, and the new phpcs:ignoreFile syntax to set ignore rules for a whole file, but I'm not sure how useful those features are and if I'll have time to do them.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Oct 30, 2017

@gsherwood Will we be able to put explanation text at the end of the line?

// phpcs:disable WordPress.VIP.SlowDBQuery.slow_db_query -- Because reason
@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 30, 2017

Will we be able to put explanation text at the end of the line?

That's not something I had considered yet, but it's a good idea. It would need some sort of separator like you've put in your comment. Have you seen that syntax used elsewhere? Anyone else have any suggestions?

@JDGrimes

This comment has been minimized.

Copy link
Contributor

JDGrimes commented Oct 30, 2017

Have you seen that syntax used elsewhere? Anyone else have any suggestions?

-- is one syntax for MySQL comments, and I think that it makes sense here. Can't think of anything better off of the top of my head.

gsherwood added a commit that referenced this issue Oct 30, 2017

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 30, 2017

Part 2 of this has been committed here: 1f07a69

The phpcs:disable and phpcs:ignore comments can now selectively ignore specific sniffs

  • E.g., // phpcs:disable Generic.Commenting.Todo.Found for a specific message
  • E.g., // phpcs:disable Generic.Commenting.Todo for a whole sniff
  • E.g., // phpcs:disable Generic.Commenting for a whole category of sniffs
  • E.g., // phpcs:disable Generic for a whole standard

Multiple sniff codes can be specified by comma seperating them

  • E.g., // phpcs:disable Generic.Commenting.Todo,PSR1.Files

You can't add your own comments yet. But you can re-enable sniffs once you've disabled them. For example:

// phpcs:disable PSR1,PSR2
echo 'PSR1 and 2 disabled here';
// phpcs:enable PSR2
echo 'PSR1 still disabled here';
// phpcs:enable
echo 'everything re-enabled here';
@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

While thinking about adding selective support for phpcs:ignoreFile I realised this isn't needed. The phpcs:ignoreFile comment tells PHPCS to quickly ignore the entire file from all checking. If you want to selectively disable rules for an entire file, you would use phpcs:disable sniff,sniff instead.

So I think the last feature to add is the ability to add your own comments.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Oct 31, 2017

It would need some sort of separator like you've put in your comment.

What if you matched one or more spaces followed by any non-word character?

Something like this: https://regex101.com/r/w1hwws/1

(could be improved a bit, but you get the idea)

Have you seen that syntax used elsewhere?

Not really, I just use it in my own projects/at work.

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

What if you matched one or more spaces followed by any non-word character?

That should be possible given the data that goes into those comments, but I'd prefer to decide on a specific syntax because it makes it easier to document and test.

@mansona

This comment has been minimized.

Copy link

mansona commented Oct 31, 2017

Hey folks, I just wanted to weigh in here on the syntax of the comments. I would really recommend that you follow the syntax for eslint because it would allow for some good overlaps.

https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments

as for the separators for "following comments", I would recommend something very simple. How about you just consider it as a list of comma separated rules which makes this valid

// eslint-disable-next-line no-console, this is more of a comment
console.log('face');

and becausethis is more of a comment doesn't match with any rule it just doesn't do anything.

Just an idea

@Frenzie

This comment has been minimized.

Copy link

Frenzie commented Oct 31, 2017

@mansona
Wouldn't you expect it to spit out some kind of warning like "unrecognized rule" in that case?

-- is also how you add comments in Lua btw. I like the // phpcs:disable WordPress.VIP.SlowDBQuery.slow_db_query -- Because reason idea.

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

How about you just consider it as a list of comma separated rules which makes this valid

The new comment syntax does use a comma separated list of rules, but PHPCS doesn't know if they are valid or not because it doesn't only have a built-in list of sniffs. Plus, it allows for single words to mean something important (disable or enable an entire standard - see comment above #604 (comment)) and it supports setting sniff config vars (phpcs:set sniff var value).

I'd like a comment syntax that supports all those formats without confusion.

I could just assume they are sniff codes and throw them into the main array. But that means having to put a comma before your comment, like this:

phpcs:disable sniffCode, because reasons

And it makes life really hard if you want to comment a "disable all" style comment, like this:

phpcs:disable Because reasons

(is Because reasons a sniff code, or should it have been phpcs:disable , Because reasons?)

I think a separator is the way to go, and I think -- is probably a good one. That's going to be an easy change, so I'll implement -- and can change it later if a better idea pops up.

gsherwood added a commit that referenced this issue Oct 31, 2017

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

I've added support for additional developer comments using the -- separator.

So you can now do this:

// phpcs:disable -- Turn off PHPCS
// phpcs:disable Generic.Commenting.Todo, PSR1.Files.SideEffects -- Because reasons

etc

You can also use block comments if you want something with more explanation. In this case, you don't need to use the comment separator because the explanation is on a different line:

/*
    We pasted this code in from a library doc and it needs to
    remain as is, so disable PHPCS for this block of code.
    phpcs:disable
*/

function Foo() {}

// phpcs:enable
@mansona

This comment has been minimized.

Copy link

mansona commented Oct 31, 2017

@Frenzie I wouldn't expect an error in this case no, this process is about turning off errors so I don't think it would be necessary.

One thing I would like to add to the wider conversation here, I use these eslint style "overrides" every day, and have been for a few years now but never have I wanted to add "extra comments" to the one line

I don't see anything wrong with the following if you really need that:

// allow for console.log here so that we can tell the user something
// eslint-disable-next-line no-console
console.log('face');

I'd much rather a simple case like this ☝️ than over-complicate the syntax.

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

I don't see anything wrong with the following if you really need that:

// allow for console.log here so that we can tell the user something
// eslint-disable-next-line no-console
console.log('face');
I'd much rather a simple case like this ☝️ than over-complicate the syntax.

You can do that with the new PHPCS syntax as well. The benefit of putting it all on one line (if it is a short message) is that the comment line itself is completely ignored by PHPCS and doesn't need to confirm to your standard's comment rules. That's not something I personally care about, but I think other developers do.

@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Oct 31, 2017

I'll throw this idea into feedback for a while, but I think this feature is now done.

@gsherwood gsherwood closed this Nov 22, 2017

rodrigoprimo added a commit to woocommerce/woocommerce that referenced this issue Nov 27, 2017

Ignore PHPCS in two instances
Ignoring PHPCS in two lines of the code where apparently it is not possible to fix the violations. I tried using what @claudiosanches suggested in this comment #17909 (comment) but both tags didn't work. Lets revisit this when PHPCS 3.2.0 is released and we can selectively disable rules (see squizlabs/PHP_CodeSniffer#604).
@o5

This comment has been minimized.

Copy link

o5 commented Dec 14, 2017

I'm using sniff Generic.Files.LineLength. I would like to disable it for specific line. It works when I place this comment above of long line. But it does not work on same line:

[
    'foo' => 'very_long_string...' , // phpcs:disable Generic.Files.LineLength.TooLong
]

In case of $lineLimit is 30 warning occurs:

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 2 | WARNING | Line exceeds 30 characters; contains 87 characters
   |         | (Generic.Files.LineLength.TooLong)
@gsherwood

This comment has been minimized.

Copy link
Member

gsherwood commented Dec 14, 2017

@o5 The // phpcs:disable syntax disable that error message from that comment onwards. If you want to ignore that line, you need to use // phpcs:ignore. Some docs here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file

Edit: I'd also suggest ignoring the whole sniff given that the really long comment is probably going to push the line length to MaxExceeded. So I'd suggest // phpcs:ignore Generic.File.LineLength

@o5

This comment has been minimized.

Copy link

o5 commented Feb 8, 2018

@gsherwood Option $ignoreComments=true for Generic.Files.LineLength resolves my issue :)

@ahmadawais

This comment has been minimized.

Copy link

ahmadawais commented Jul 16, 2018

This issue shows up a lot in the search so I just wanted to share this as a result of this discussion.

The @codingStandards syntax is deprecated as of v3.2.0, use phpcs instead.

Some examples:

For Multi-line disable

// phpcs:disable
// phpcs:enable

For Single line ignore

// phpcs:ignore

See this page for more information: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file

Peace! ✌️

@flobee

This comment has been minimized.

Copy link

flobee commented Sep 24, 2018

Gooood feature!
One hint: Hints for the editors needed to search and select for eg: @phpcs<strg+space> :) like it works in phpunit apigen Codeception ... and others. Or does it exists and i dont know how?

I prefer using @ annotations which works like:

/**
 * ....
 * @phpcs:ignoreFile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.