-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Allow FQCN as fixer name and configuration key for rules #7064
base: master
Are you sure you want to change the base?
feat: Allow FQCN as fixer name and configuration key for rules #7064
Conversation
maybe do not allow custom rule name start with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's fine, but I'd love to hear from @keradus about this change as the naming come from ancient times of fabpot/php-cs-fixer
that only he remembers :)
sorry, not sure if I pick up the question around fabpot/* namespace, can you clarify? |
@keradus I believe @kubawerlos just wonders if you're in favour of such change or maybe there were some ancient decisions about naming convention that you wouldn't like to change 😉. |
tests/FixerNameValidatorTest.php
Outdated
['Vendor/foo', true, true], | ||
['Vendor4/foo', true, true], | ||
['4vendor/foo', true, false], | ||
['Vendor/foo', true, true], | ||
['FooBar/foo', true, true], | ||
['foo\\bar', true, true], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original idea was to support format of vendor/rule
. now you also add vendor\rule
.
not sure if I would like to keep 2, /
and \
, alternative syntaxes
I would like to keep explicit vendor/
part - and then whatever after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we expose our integration point with interface and we expect specific format, ie vendor/rule
, and some external library decides to not use that format, I would rather expect that format that is expected is provided, rather than we change expectations.
^^ that's about the principle approach in general.
point 1
for this case specifically, i would love to see have single way how we format of rules. and if it's sometimes foo/bar
, and sometimes foo\bar
, then it's off and I would love to avoid it.
point 2
if we consider to touch possible format of name for custom rules, i would suggest to combine the alignment with possible format of name for custom rulesets
point 3
if using random string is a drawback, let's solve that drawback properly - change of string format is not solving them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we expose our integration point with interface and we expect specific format, ie vendor/rule, and some external library decides to not use that format, I would rather expect that format that is expected is provided, rather than we change expectations.
This PR is not created in order to align with userland convention, but to address several concerns raised in #7062. So this is not about changing expectation, rather about changing general approach. Using FQCNs enables several advantages - code navigation, usage detection, visual deprecation indicator, easy refactoring etc.
In terms of other points:
- I think this can be only for transitional period, because I think FQCN would be better as a contract in the future. At this point we can allow both and systematically, step by step, shift in FQCN direction.
vendor/name
is kept for backward compatibility (allows only 1/something
segment), while newly introducedvendor\name
allow 1+\something
segment(s) and is forward-compatible. - Yes, we could allow FQCNs as a name of custom rule sets too.
- Not sure what you mean. We can't solve it just like that, as I wrote in the point 1 - it requires transition period. Allowing FQCNs is first step of the process which may lead to full switch to FQCNs in v4.
PS. Support for FQCNs for built-in fixers can be done with 2 lines, by adding $this->fixersByName[get_class($fixer)] = $fixer;
to FixerFactory::registerFixer()
and aligning $availableFixers
in ConfigurationResolver::validateRules()
. I could even add it in this PR so people can instantly start configuring Fixer using FQCNs, at least when it comes to fixers, not rulesets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, but for now I'm not convinced to use FQCN to setup fixers.
Why can't we allow it at least, though? I would also welcome it, to make it compatible with 3rd party fixers. |
@keradus I'be been using FQCN for years in ECS and I find it better because of reasons mentioned in the #7062 and here. It simplifies so much, you can easily check if provided FQCN refers to class that implements required contract, no need for registration flow etc. I don't get why you don't see those advantages... |
11b962e
to
8e1a7a7
Compare
8e1a7a7
to
08dd546
Compare
08dd546
to
cdfb23b
Compare
Full support for FQCN added, needs new review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marking as blocked to prevent too-eagerly-fast merge.
I want to think over this first and ensure it's approached in direction we want to go long-run. Eg:
- we should have same approach for rule and for ruleset
- one of long term idea was to make config code-less and have static file (eg JSON). This direction is going to support config as code, that I would rather sunset for long-run
I treat it more like RFC for now, didn't check the implementation
It was already discussed - I agree, but it won't happen in this PR. It can be added later as a part of a longer plan (just like custom rulesets). |
I do not recall any alignment on direction for those elements I mentioned. Maybe because tight year (for me) and lack of capacity as high as yours. |
Take a look here (and your comment above), I already addressed your concerns about long-term vision and consistency between rules and rulesets. |
I understand that you have the vision and that we didn't had a chance to get enough attention from my side to conclude on it - and while you have idea, I have no conclusion. For me it was definitely 8.3. It's done for few days, time to recover and then prioritise topics to focus on further (opening more till we conclude on previous not helping me). |
ab3bc23
to
375b6e9
Compare
I think it is a really good move! I would like to use FQCNs for built-in rules, rule-sets and custom rules. The old names should be marked as deprecated in my opinion. Therefore, in the long term, it should only be possible to configure the rules via the FQCNs. |
375b6e9
to
a9f5b32
Compare
- Allow FQCN as fixer name - Allow lowercased vendor in custom fixers (foo/bar)
It's fully backward compatible, but allows using FQCNs as a keys in rules' config, instead of short names. This enables code navigation, usage discoverability etc.
a9f5b32
to
601cb1f
Compare
Fixes #7062