PHPCSStandards / PHP_CodeSniffer

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

Feature Request: Per-directory configuration #126

Open anomiex opened 9 months ago

anomiex commented 9 months ago

Is your feature request related to a problem?

It's difficult to write and maintain XML configuration rules to apply different subdirectories, as the existing configuration file syntax is oriented towards specifying paths that apply to rules rather than rules that apply to paths.

See also https://github.com/squizlabs/PHP_CodeSniffer/issues/2551

Describe the solution you'd like

Per-directory configuration files would be nice. Conceptually there are at least two ways this might work: for example, if you have a file at subdir/.phpcs.dir.xml then for files in that directory phpcs would

  1. act as if it had been run as phpcs --standard=.phpcs.xml.dist,subdir/.phpcs.dir.xml, loading the subdirectory ruleset as a peer of the parent's, or
  2. use subdir/.phpcs.dir.xml as the ruleset, but treat that file as if it had <rule ref="../.phpcs.xml.dist" /> at the start.

Additional context (optional)

We've been using a fork of phpcs that enables this in our monorepo at https://github.com/Automattic/jetpack since December 2021 and it has worked out very well for us despite some caveats.

The important caveats with the version in the fork are:

Since a 4.0 release seems imminent in the new repo, I wonder whether I might do better to do a more invasive PR than we had done in https://github.com/squizlabs/PHP_CodeSniffer/pull/3378. The main breaking change a more invasive PR would do would be to change methods like Config::getConfigData() to be non-static, which would affect sniffs that use Config. Probably they'd have to access the instance via $phpcsFile->config instead.

jrfnl commented 9 months ago

I'm not sure I fully understand your problem.

You can already use option 2 by having an explicit <rule ref="../.phpcs.xml.dist" /> at the start of the subdirectory ruleset. Making that implied will be a huge breaking change which I'm not willing to make.

Having it explicit is a one-liner, so why is anything else needed ? (aside from the overruling issue, but that would be solved once 4.0 is released).

Since a 4.0 release seems imminent in the new repo, I wonder whether I might do better to do a more invasive PR than we had done in https://github.com/squizlabs/PHP_CodeSniffer/pull/3378. The main breaking change a more invasive PR would do would be to change methods like Config::getConfigData() to be non-static, which would affect sniffs that use Config. Probably they'd have to access the instance via $phpcsFile->config instead.

That is not a change I would accept for the 4.0 release. To be able to get to 4.0 sooner rather than later, I want to stabilize things, not de-stabilize by making larger breaking changes which I cannot oversee the full impact of at this time.

anomiex commented 9 months ago

I think the part you're missing is that I want to run phpcs just once at the root of the project, and then for some/path/ that has some/path/.phpcs.dir.xml it would transparently use that standard for files under that path, and for another/path/ with a different another/path/.phpcs.dir.xml it would use that standard for files in there, and for another/path/and/deeper/ with another/path/and/deeper/.phpcs.dir.xml it would use that standard (which itself "extends" another/path/.phpcs.dir.xml) for files there.

Yes, you can get something close to this with configuration like you suggest and multiple runs of phpcs and careful exclusion of the files in the higher-level runs that will be covered in the subdirectory runs instead. And then possibly having to merge together the results from the multiple runs for CI tooling to make use of. But all that is a pain and hard to scale.

Note the implied <rule ref="../.phpcs.xml.dist" /> would only apply to these per-directory .phpcs.dir.xml config files. If someone is already using the multiple-runs method instead, those wouldn't be per-directory config files and so wouldn't have the implied <rule>. And I'm still leaning towards there being no default name for the per-directory files, so anyone wanting to use it would have to opt-in by setting a name for them in their configuration.

I do like the implied <rule> for the per-dir files, because then you don't have to worry keeping them in sync when changing things around. Say you were to decide to add another/.phpcs.dir.xml, for example: with an explicit <rule> you'd have remember to update another/path/.phpcs.dir.xml (and any others) to point to that now instead of the .phpcs.xml.dist in the root.

That is not a change I would accept for the 4.0 release.

Thanks for letting me know. Would 5.0 be expected to happen reasonably soon after 4.0 (like months to a year rather than multiple years) and a breaking PR would be accepted for that? Or would you rather have a rebase of https://github.com/squizlabs/PHP_CodeSniffer/pull/3378, since that was carefully designed to not break anything?

jrfnl commented 9 months ago

I think the part you're missing is that I want to run phpcs just once at the root of the project, and then for some/path/ that has some/path/.phpcs.dir.xml it would transparently use that standard for files under that path, and for another/path/ with a different another/path/.phpcs.dir.xml it would use that standard for files in there, and for another/path/and/deeper/ with another/path/and/deeper/.phpcs.dir.xml it would use that standard (which itself "extends" another/path/.phpcs.dir.xml) for files there.

Indeed the bit I was missing. Will need to have a think about this, but this is not something which is on my priority list at this time.

That is not a change I would accept for the 4.0 release.

Thanks for letting me know. Would 5.0 be expected to happen reasonably soon after 4.0 (like months to a year rather than multiple years) and a breaking PR would be accepted for that?

I do expect to have more regular majors, once a year/once every two years.

However, my focus at this time is on stabilizing and on getting to 4.0 and then on some structural changes I already have planned for 5.0 myself. I currently don't have the bandwidth to make a judgement call on this ticket for 5.0. I will need to circle back to this at a later time.