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

Feature Request: Per-directory configuration #126

Open
1 task done
anomiex opened this issue Dec 4, 2023 · 3 comments
Open
1 task done

Feature Request: Per-directory configuration #126

anomiex opened this issue Dec 4, 2023 · 3 comments

Comments

@anomiex
Copy link
Contributor

anomiex commented Dec 4, 2023

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 squizlabs/PHP_CodeSniffer#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:

  • The fork itself is pretty minimal, as in the old repo it seemed like 4.0 was never going to happen. Basically the fork just enables filters to return a PHP_CodeSniffer\File instance instead of a filename. Then a custom filter returns File instances that have different values for ->ruleset and ->config as necessary to reflect the per-dir config files.
  • Since the PHP_CodeSniffer\Config class is half-static, <config> doesn't work well at a per-directory level.
    • Various other directives like <ini> and <arg> and <autoload> also don't work at a per-directory level, but those are much easier to classify as "global" and therefore say they shouldn't work per-directory.
  • Since our fork takes the --standard=.phpcs.xml.dist,subdir/.phpcs.dir.xml approach rather than the implicit-<rule ref="../.phpcs.xml.dist" /> approach (because the latter is a lot harder to do from a filter), <exclude name="Standard.Category.Rule" /> doesn't work to exclude a rule that had been enabled in a parent directory. Since <exclude name="Standard.Category.Rule"><severity>0</severity></exclude> does still work, that hasn't been that much of a problem.

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 squizlabs/PHP_CodeSniffer#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.

  • I intend to create a pull request to implement this feature.
@anomiex anomiex changed the title Per-directory configuration Feature Request: Per-directory configuration Dec 4, 2023
@jrfnl
Copy link
Member

jrfnl commented Dec 10, 2023

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 squizlabs/PHP_CodeSniffer#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
Copy link
Contributor Author

anomiex commented Dec 10, 2023

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 squizlabs/PHP_CodeSniffer#3378, since that was carefully designed to not break anything?

@jrfnl
Copy link
Member

jrfnl commented Dec 10, 2023

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.

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

2 participants