DX: re-apply CS #53233
DX: re-apply CS #53233
Conversation
keradus
commented
Dec 27, 2023
keradus
commented
| Q | A |
|---|---|
| Branch? | 7.1 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | no |
| Issues | Fix CS |
| License | MIT |
| @@ -420,9 +420,9 @@ public function addArgument(string $name, int $mode = null, string $description | |||
| /** | |||
| * Adds an option. | |||
There was a problem hiding this comment.
overall, we got curious if phpdoc_align make sense on Fixer side, especially because of such cases like here. WDYT?
The version proposed by PR is one in-line with current CS rules.
There was a problem hiding this comment.
I agree it might be better to not align those
There was a problem hiding this comment.
I would love to have conclusion that is "always align" or "never align".
If we say always, let's pay this little price of having this single case out of 100k phpdocs ugly. Maintaining exception and not letting tool to do it's job for single case is painful and preventing auto-apply of tool results.
There was a problem hiding this comment.
I don't think the current changes on this are improving the source. Consistency is nice, but this goes against readability here to me.
I understand you'd like a rule, so maybe we could just decide which length is too much to align? or say that complex types shouldn't trigger alignment?
we can revert these from the PR and discuss that separately if you don't want to other changes to wait for this to be resolved
There was a problem hiding this comment.
for 6k files in this repo, we have the rule not applied for TWO cases only. instead of making some special handling of this and implementing rule for exception, I would ask to get this applied. Let's be pragmatic here and avoid increasing rule complexity for such rare case.
There was a problem hiding this comment.
we accept exceptions so it's fine have two cases that don't follow the rule
please revert the related changes in this PR and 🚀
There was a problem hiding this comment.
It makes it tricky to apply the CS (and on long run, have CS always green) - because manually reverting the exceptions each time :(
reverting as requested
| @@ -184,7 +184,7 @@ public function getClassLoader(): callable | |||
| public static function enable(): void | |||
| { | |||
| // Ensures we don't hit https://bugs.php.net/42098 | |||
| class_exists(\Symfony\Component\ErrorHandler\ErrorHandler::class); | |||
| class_exists(ErrorHandler::class); | |||
| class_exists(\Psr\Log\LogLevel::class); | |||
There was a problem hiding this comment.
Should also be imported, right?
There was a problem hiding this comment.
In this PR, I applied the fully_qualified_strict_types rule, which (with current config) simplifies usage of symbols.
If you want to import symbols, we can enable import_symbols.
Please decide if you would like to go this path as separated PR: #53244
src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php
Outdated
Show resolved
Hide resolved
|
(rebased due to fabbot.io request) |
nicolas-grekas
force-pushed
the
7.1_CS
branch
from
e1befbd
to
233110a
Compare
|
Thank you @keradus. |
| @@ -24,7 +24,7 @@ trait OidcTrait | |||
| { | |||
| private function createUser(array $claims): OidcUser | |||
| { | |||
| if (!\function_exists(\Symfony\Component\String\u::class)) { | |||
| if (!\function_exists(u::class)) { | |||
There was a problem hiding this comment.
we need the namespace to check for the existence of the function (fixed in 862c6fb)
There was a problem hiding this comment.
good catch, let me convert it into bug report:
PHP-CS-Fixer/PHP-CS-Fixer#7652
There was a problem hiding this comment.
I find this very curious inconsistency on PHP engine side.
There was a problem hiding this comment.
deserves a bug report to php-src IMHO
There was a problem hiding this comment.
I see reason why PHP allows for that. For ::class, one can do whatever, like fooisufdhsilufgvhlaiuwrgfliudfzbhsdfbsdfb::class, and it's not evaluated if that foo..sth exists at all, therefore also not if it's a class or any other symbol like function.
It would need the compiler to already have execution context, or at least import all files... to analyse. IMHO doubtful to get in, but if you think otherwise and have better expertise on it, please consider to drive this report.
