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

Add .phpcsignore file #1884

Open rmccue opened 6 years ago

rmccue commented 6 years ago

It would be super useful if there was a way to ignore files and directories without touching the PHPCS standard file. This would be similar to tools like ESLint, which has a .eslintignore file separate to the standard.

For our specific use case, we want to share a standard across many projects, but those projects might include external libraries or legacy code that need to be ignored on a project-basis. Being able to do this without needing to extend the standard would make adoption across our legacy projects much easier.

rmccue commented 6 years ago

(It's already possible to do this via --ignore of course, but in our setup, the command is run for the repos centrally so that can't be controlled.)

gsherwood commented 6 years ago

You can include one standard inside another. Not sure if it helps your situation, but it's the way I've envisaged projects sharing a standard but also bringing in some project-specific settings. There are some examples in the wiki.

If we can find a small tweak to make the ruleset approach work for you, it's more likely to make it into a release than another method of ignoring files.

rmccue commented 6 years ago

We've been using this method of ignoring files, but it's been quite cumbersome to work with for developers who aren't super familiar with phpcs. It's also a bit of a pain to do the full ruleset XML setup just to ignore a file or two; I imagine this is the same reason ESLint supports a separate file.

That said, totally understand if you don't want to add it to phpcs. I guess the question then becomes: can we implement this in our own code? I'd be happy to build this out inside our bootstrap file if it's possible to alter the configuration at that point.

(Alternatively, we could parse the file ourselves and build a --ignore parameter from that, but I'd rather avoid that if we can.)

photodude commented 6 years ago

@rmccue See how we set it up for Joomla following the wiki instructions. (I believe WordPress is also doing this) you basically use a ruleset for Selectively Applying your standard it's a separate file and it modifies things for each project https://github.com/joomla/coding-standards#selectively-applying-rules

rmccue commented 6 years ago

Yep, I’m well-aware of the full-configuration method, but my aim is to reduce the friction of adopting the full ruleset across our projects. Writing a ruleset file is annoying for developers who just want to ignore one file, whereas writing an ignore file is super simple, and already familiar to most developers from things like gitignore.

photodude commented 6 years ago

I don't understand how a few lines of XML to extend an existing full ruleset is overly complicated

This is all you need to exclude a single file in a project that wouldn't be excluded by the full standard.

<?xml version="1.0"?>
<ruleset name="Joomla-Stats">
    <!-- Use our Full Standard -->
    <rule ref="Joomla" />
    <!-- Exclude the RoboFile.php -->
    <exclude-pattern type="relative">RoboFile.php</exclude-pattern>
</ruleset>

Sure a single line in a single file is "less complicated" but the example above is still very clear and reasonably simple.

rmccue commented 6 years ago

The issue is more that you have to learn a domain-specific language (ruleset XML) to exclude a file. That's fine if you're working with phpcs all the time, but for developers who don't need or want to learn that, it's better if they can use something they already know. Ignore files are reasonably standard on the other hand.

We're trying to make the use of phpcs easier across the board for all our projects, and simplifying this is the biggest remaining item we have to tackle there. Like I said, totally understand if it's not something other people want, but we're going to implement this in one form or another. :)

rmccue commented 6 years ago

Here's my preliminary pass at this. I'm loading this in via the <autoload /> config directive in our coding standard, since AFAICT there's no way to specify bootstrap files from the standard without potentially overriding an actual command-line parameter.

I'll work on rolling this out to our engineers first as a test user base, and once we've fixed any issues, I'll look at sending a PR to integrate this into phpcs proper.

jrfnl commented 6 years ago

@rmccue While not exactly what you are looking for, the opposite is already available through the --file-list option.

--file-list=\<fileList> \<fileList> A file containing a list of files and/or directories to check (one per line)

See: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Usage

It does mean you have to pass the argument pointing to the file on the command-line (or set it in the custom ruleset), but also that your devs who aren't familiar with PHPCS configurations, don't have to be as they can just maintain this simple file list instead.

Does that help ?

rmccue commented 6 years ago

@jrfnl --file-list is an inclusion list, rather than an exclusion list. With codebases of the size we're working on, such a list would be prohibitively big, and would fall out of date pretty quickly.

We also have phpcs configured at an organisational level, so devs can't pass custom parameters.

We're running .phpcsignore right now via https://github.com/humanmade/coding-standards/pull/39; I don't have any stats on how many projects are using it yet, but I can investigate that if you're interested, and turn it into a PR for phpcs proper.

jrfnl commented 6 years ago

@rmccue That's why I said it was the opposite ;-)

All the same, whether you are maintaining an inclusion list or an exclusion list when adopting CS for a legacy project should be the same amount of effort, just for the one most effort would be placed at the beginning (exclusion), while for the other towards the end (inclusion).

rmccue commented 6 years ago

All the same, whether you are maintaining an inclusion list or an exclusion list when adopting CS for a legacy project should be the same amount of effort

These are greenfields projects and are brand new (relatively), so I wouldn't call them "legacy". They do use external libraries though that will probably never match our coding standards, so we need to ignore them.

Imagine if vendor wasn't excluded by default in phpcs. It would make the most sense to then have a .phpcsignore file with vendor/ in it, rather than a .phpcsinclude (e.g.) with every file you want to check in it.

Inclusion lists are really a non-starter for us. Having to enumerate every file in the project simply to exclude one directory is a lot of effort, and fundamentally doesn't make sense. It would make the default state of a file ignored, which means it's now much easier for files to be accidentally excluded.

just for the one most effort would be placed at the beginning (exclusion), while for the other towards the end (inclusion)

I'd disagree with this too; for both, the effort is whenever new things are added, and the decision is made at that point. The difference is that an inclusion list means the burden exists for code that actually follows the standards, whereas exclusion means the burden is for code that doesn't.

Even for "legacy" projects that are adding CS, listing things out makes the most sense when listing things that are exceptional. The goal should be to get your list down to zero, not to gradually increase it until it includes every file, then delete it.

jrfnl commented 6 years ago

These are greenfields projects and are brand new (relatively), so I wouldn't call them "legacy".

@rmccue Never mind. I guess I misread your original topic:

make adoption across our legacy projects much easier

Imagine if vendor wasn't excluded by default in phpcs.

It's not (excluded by default). You may have set that in your custom ruleset, but it's not a PHPCS native thing.

It would make the most sense to then have a .phpcsignore file with vendor/ in it, rather than a .phpcsinclude (e.g.) with every file you want to check in it.

Not necessarily, as the include list allows for directories too, so I don't see the difference between having an ignore file with vendor/ in it or an include list with src/ in it.

See my original reply: https://github.com/squizlabs/PHP_CodeSniffer/issues/1884#issuecomment-431135929

Anyway, sounds like you're unwilling to consider any other solution than the original proposal, so I'll leave you to it.

alfredbez commented 4 years ago

We use something like this:

$ phpcs --ignore=$(paste -s -d, .phpcsignore) .

The .phpcsignore looks like this:

$ cat .phpcsignore
vendor
*/tmp/*
kadamwhite commented 4 years ago

Disclaimer: I work with @rmccue. But I'd like to come and bump this issue, because I just helped two different teams with large PHP codebases fix issues where they'd improperly excluded things via the XML system. Having a clear syntax like this for an ignore file would bring PHPCS closer to Git, ESLint, and all the other tools used in a modern web project, and would save us a lot of time helping clients fix errors. XML shouldn't be too hard, but I do still see mistakes with the exclude-pattern option on a regular basis.

Strong :heavy_check_mark: for this enhancement :crossed_fingers:

anomiex commented 3 years ago

I'm surprised no one has pointed at PHPCS's --filter option for this yet. I found it pretty easy to implement .phpcsignore as a filter; the hardest part was that there is no (documented) PHP library for dealing with ignore-format files so I had to write that too if I wanted it to work like .gitignore rather than having a similar-but-different syntax. So kudos to PHPCS for having the filter option!

My filter isn't ready to be released yet because it also uses the change to PHPCS that I proposed in #3378 to give us per-directory configuration that we also want for our monorepo. Once that is merged (or some other way to accomplish per-directory configs is made available) we'll publish it to packagist.

Off-topic discussion of other options. I find two problems with ``. 1. The syntax isn't documented beyond a few examples. The examples make it out to be some sort of globbing (but which sort?), while really it's a mangled PCRE regex. It's also unclear exactly what it matches against, or how exactly `type="relative"` changes that. 2. It has to be put in (or included by) the single root XML file, where a properly done `.phpcsignore` could be added in subdirectories too just like `.gitignore`. FWIW, `.eslintignore` has this problem too. `--ignore` has the same drawbacks as ``. `--file-list` works well for its purpose, but the fact that it's a list of files to process rather than a list of patterns to exclude means that you either have to generate the list dynamically, or have newly-added files not get processed by default, or design your directory structure around whether files should be checked or not.
stracker-phil commented 3 years ago

I'm also looking for some kind of .phpcsignore file to exclude third-party code from validation.

We develop software for a partner platform and have the following constraints:

  1. The PHPCS rules are defined by the platform, we have no means to exclude "vendor" via the configuration.
  2. Code validation is done on a remote server, so I cannot pass any CLI parameters to phpcs either.
  3. Before my project is accepted, my entire repo must pass the PHPCS validation.
  4. I can only add inline-annotation into code to mark certain statements as "intentional and save" via // phpcs:ignore

Currently, I need to manually add hundreds of // phpcs:ignore statements into third-party library files to pass the code validation.

Instead, the process would be much faster, safer and transparent by adding a .phpcsignore file that tells PHPCS to ignore "vendor" files; ideally, also with a comment # third-party libraries: https://url-to-library-repo

MurzNN commented 10 months ago

Got the same problem with missing an ability to ignore specific folder from phpcs scanning. I'm developing a sub-module for a project, that will be scanned using general rules from the project. And I need to just mark some directories to skip producing code style errors in them.

So, the .phpcsignore file will be a great improvement!

jrfnl commented 10 months ago

@MurzNN You are aware of the <exclude-pattern> directive and the CLI --ignore arg ? Both should work for your usecase.

MurzNN commented 10 months ago

@jrfnl Yes, I'm aware, but I can't ask the project to modify their CI pipelines personally for my sub-module. I can only push changes to my own sub-module directory.