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

feat: Support external rulesets #6083

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

niklam
Copy link
Contributor

@niklam niklam commented Oct 29, 2021

Supporting external rule sets makes it easier to share rule sets across different projects. In many cases a single product is spread across multiple repositories, so this is important.

Registering a RuleSet is done with ConfigInterface::registerRuleSet(array $ruleSets).

Actual rule set addition is done in RuleSets, where rule set name and class are validated against certain rules:

  • Name must contain only letters (a-z, A-Z), numbers, underscores, slashes, dots, hyphens, and it must begin with "@"
  • Name must not overlap with pre-defined rule set
  • Class must exists (doh!)
  • Class must implement class RuleSetDescriptionInterface

Sample usage:

    $config = new PhpCsFixer\Config();
    return $config
        ->registerCustomRuleSets([
            '@MyRuleSet' => \MyNameSpace\MyRuleSetClass::class,
        ])
        ->setRules([
            '@MyRuleSet' => true,
        ])
    ;

Note
This is not a breaking change as ConfigInterface is not changed while this introduces a new method to Config. The new method should be added to the interface in version 4.0.

TODO

@coveralls
Copy link

coveralls commented Oct 29, 2021

Coverage Status

Coverage increased (+0.007%) to 93.178% when pulling 1433083 on niklam:feature-support-external-rulesets into b0a9fca on FriendsOfPHP:master.

@SpacePossum
Copy link
Contributor

SpacePossum commented Oct 29, 2021

Thanks for all your work on this idea!

I've not time now to look into some more right now, however I think #5802 is aiming for something similar so cross referencing it here

@niklam
Copy link
Contributor Author

niklam commented Oct 29, 2021

I tried to search for implementations for this and failed to find it. On my own comparison now I see that the other PR changes logic further than mine. Creating a factory (which I would rather call a repository) for these is not needed in my opinion, but it's a nice idea if we want to start serving RuleSets from different places (databases, different formats).

To me my way of naming the RuleSets is easier to understand, but this is matter of taste, too.

If you decide this is a way to go, I'm happy to improve documentation for this prior to merge.

I will not change this further until receiving comments.

@niklam
Copy link
Contributor Author

niklam commented Nov 6, 2021

@SpacePossum, as discussed in another PR, for this to be really usable the RuleSetDescriptionInterface should likely be made public. Would you prefer another PR or would be in scope of this PR in your opinion?

Another question would be if you'd prefer Cache to store the new rulesets as name => FQCN array and process them later, or keep doing what it's doing right now. I kinda would like latter behaviour more as it allows to catch the possible error where it happens, but then again it's not consistent with how custom fixers seems to behave.

@keradus
Copy link
Member

keradus commented Nov 10, 2021

can we compare #6083 and #5802 approaches and align on best design?

@JPustkuchen
Copy link

Really looking forward to this for Drupal to make it usable like for phpcs (--standard=DrupalPractice) or phpcbf! (--standard=Drupal)
See https://packagist.org/packages/drupal/coder Thank you so much for making this possible!

@SpacePossum
Copy link
Contributor

phpcs is another tool, not this one @JPustkuchen ;) but we are happy to welcome the Drupal community here :D

@JPustkuchen
Copy link

JPustkuchen commented Nov 26, 2021

Thank you @SpacePossum! Sorry, I know. Perhaps I didn't choose the right words. I just wanted to point out that there are similar funtionalities for phpcs and phpcbf and that page documenteds, how to set the standards for these tools Drupal-specific. I'd be happy if one day php-cs-fixer could appear there and could work like that. Sorry for the misunderstanding.

Once this is finished, I'll document it over there for the Drupal Community.

@SpacePossum
Copy link
Contributor

Ah sorry about that, all good :D Thanks for looking out for this feature and the tool itself!

@DaanBiesterbos
Copy link

This would be exactly what I was looking for. Any idea if and when this would be added?
We'd like to have a central repository with the basic rulesets of our company that can be extended within teams or application (e.g. legacy vs new code for example).

I was kind of surprised that this was not yet possible so I think your PR is spot on. Great work 👍

@niklam
Copy link
Contributor Author

niklam commented Nov 26, 2021

Author here. I think that after @SpacePossum and others have decided if the approach taken to tackle the issue is what should be done, I still need to write proper documentation for the feature.

It should be noted that this is a breaking change as this changes a public interface ConfigInterface, so in my opinion this can't be rolled out as a minor change. At the same time I don't personally see an issue in releasing a new major version, but there's likely a lot of things about this project I don't know and why that might be a problem.

@niklam
Copy link
Contributor Author

niklam commented Nov 29, 2021

@SpacePossum what should we do to get this moving to one direction or other? 🙂

@niklam
Copy link
Contributor Author

niklam commented Jan 25, 2022

Anything I can do to get this forward? I would need the feature myself. 😄

@niklam niklam force-pushed the feature-support-external-rulesets branch from cf05ddb to 1433083 Compare January 29, 2022 09:48
@SpacePossum
Copy link
Contributor

I'll put this on the list to discuss within the team in a google call (likely early next week), hope we come to a plan to more forward, thanks for understanding :)

@niklam
Copy link
Contributor Author

niklam commented Feb 2, 2022

Thanks for the update! 🙂

@github-actions

This comment was marked as outdated.

@Wirone
Copy link
Member

Wirone commented Jun 4, 2023

@niklam is it possible to rebase this PR, so we can check current pipeline result and continue the review process?

@niklam
Copy link
Contributor Author

niklam commented Jun 4, 2023

@niklam is it possible to rebase this PR, so we can check current pipeline result and continue the review process?

Sure 👍

@niklam niklam force-pushed the feature-support-external-rulesets branch from 1433083 to d71c931 Compare June 4, 2023 15:50
@Wirone Wirone changed the title Feature: Support external rulesets feature: Support external rulesets Jun 4, 2023
@Wirone
Copy link
Member

Wirone commented Jun 4, 2023

Hmmm, needs some tweak here and there 😅.

@niklam
Copy link
Contributor Author

niklam commented Jun 4, 2023

Hmmm, needs some tweak here and there 😅.

Yes 😅 A lot has happened between my PR and now. Should be good now!

@github-actions github-actions bot added status/to verify issue needs to be confirmed or analysed to continue and removed status/stale labels Jun 5, 2023
niklam and others added 25 commits May 20, 2024 23:28
This allows to do the same sorting across multiple places where it's needed, currently ::getSetDefinitions() and ::registerRuleSet().
Co-authored-by: Greg Korba <wirone@gmail.com>
Co-authored-by: Kuba Werłos <werlos@gmail.com>
Co-authored-by: Greg Korba <wirone@gmail.com>
- cover built-in rule sets
- cover risky convention
It does not make sense to pass name explicitly during registration, because ruleset should identify itself using contract (`RuleSetDescriptionInterface::getName()`).
@Wirone Wirone force-pushed the feature-support-external-rulesets branch from 780c403 to 81c7a5a Compare May 20, 2024 23:29
@Wirone
Copy link
Member

Wirone commented May 20, 2024

@keradus I've reworked how custom rules sets' registration is handled, also added TODO section to PR's description with links to ongoing discussions that have to be resolved. It would be great if we can continue this, because I see it as the next most important improvement after parallel runner 🙂.

@Wirone Wirone dismissed stale reviews from kubawerlos, keradus, and themself May 20, 2024 23:49

Outdated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants