feat: Require list of fixers to be a list separated by new lines #7370
Conversation
localheinz
force-pushed
the
feature/list
branch
from
0c5fad4
to
83d6183
Compare
| * Must run before: | ||
| * | ||
| * - GeneralPhpdocAnnotationRemoveFixer | ||
| * - GeneralPhpdocTagRenameFixer | ||
| * - NoBlankLinesAfterPhpdocFixer | ||
| * - NoEmptyPhpdocFixer | ||
| * - NoSuperfluousPhpdocTagsFixer | ||
| * - PhpdocAddMissingParamAnnotationFixer | ||
| * - PhpdocAlignFixer | ||
| * - PhpdocAnnotationWithoutDotFixer | ||
| * - PhpdocInlineTagNormalizerFixer | ||
| * - PhpdocLineSpanFixer | ||
| * - PhpdocNoAccessFixer | ||
| * - PhpdocNoAliasTagFixer | ||
| * - PhpdocNoEmptyReturnFixer | ||
| * - PhpdocNoPackageFixer | ||
| * - PhpdocNoUselessInheritdocFixer | ||
| * - PhpdocOrderByValueFixer | ||
| * - PhpdocOrderFixer | ||
| * - PhpdocParamOrderFixer | ||
| * - PhpdocReturnSelfReferenceFixer | ||
| * - PhpdocSeparationFixer | ||
| * - PhpdocSingleLineVarSpacingFixer | ||
| * - PhpdocSummaryFixer | ||
| * - PhpdocTagCasingFixer | ||
| * - PhpdocTagTypeFixer | ||
| * - PhpdocToCommentFixer | ||
| * - PhpdocToParamTypeFixer | ||
| * - PhpdocToPropertyTypeFixer | ||
| * - PhpdocToReturnTypeFixer | ||
| * - PhpdocTrimConsecutiveBlankLineSeparationFixer | ||
| * - PhpdocTrimFixer | ||
| * - PhpdocTypesOrderFixer | ||
| * - PhpdocVarAnnotationCorrectOrderFixer | ||
| * - PhpdocVarWithoutNameFixer |
There was a problem hiding this comment.
not sure if it's easier now :Sweat:
There was a problem hiding this comment.
we have 2 types of priority dependencies. One, declared explicitly in (getFixersPriorityGraph), for which we are creating integration tests, and other one, declared implicitly (getPhpDocFixersPriorityGraph, ref https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/tests/AutoReview/FixerFactoryTest.php#L940 ) - maybe it would be interesting to distinguish this in the docblock?
I imagine "explicit" list is sth important for the docblock, and the "implicit phpdoc priorities" are good to know, but you can kind-of ignore them while reading
There was a problem hiding this comment.
and of course we have getFixerWithFixedPosition, that i think we do not even document here
There was a problem hiding this comment.
Maybe we should have an internal fixer that adds these DocBlocks?
There was a problem hiding this comment.
We have test that tests it - I'm OK with any approach, but I'm not sure if there is biggest value in automating applying the format. Oh wait, there is - because you want to change the format ;)
I think first things first - decide on format itself. We can come up with any format, the question is how useful this doc is.
Maybe we just dump the graph to single file (autogenerated), and we link that file here - random example that it does not need to be phpdoc+fixer.
myself, I do not rely on this comment at all - originally it was awesome to have it, but now it's that complex and long, that I actually skip the comment totally.
There was a problem hiding this comment.
there was some good discussion somewhere that it's nice to have it as code, but also it will be BC break for custom fixers that are relying on numbers (potentially we can have a transition period when both methods returning same results).
I believe we could utilise current priorities for building dependency graph too, just by treating all fixers with lower priority as "dependencies" of fixers with higher priority. It would create huge list, but in the end it could be simplified to proper ordered list. I think the requirement for having integration test between fixers should be defined only by explicit dependency (using AbstractFixer::getPrecedingFixers()), so during implementation of such system we should migrate current dependency graph to the fixers' methods. This way we could keep support for custom fixers' priorities, but at the same time introduce new approach.
So, for transition period it would work like this:
- get "dependant" fixers by priority (all fixers with lower priority)
- get explicit dependencies defined in
AbstractFixer::getPrecedingFixers() - merge those lists (keep only unique values)
- calculate actual order of running, so each fixers is run after all dependant fixers
Then, in next major, we could drop support for priorities completely.
Current approach based on priorities is not good because:
- it's cumbersome to add new fixers between existing fixers when there's no unused priority between existing fixers (it requires changing priority to many other fixers just to move all of them up/down)
- term "priority" is not actually what it is. "Priority" for me relates to importance, but all fixers are equally important, they just need to be run in proper order 😉.
In the future, fixers' order resolver could also introduce support for pre/post hooks, that would enable running some fixers multiple times for each file (once in calculated order, but also as a post-hook for other fixer that is applied later), which would completely resolve problems that currently happen, when user need to run Fixer more than once to get the final result. Maybe automatic order calculation would be efficient enough, but having it flexible and dynamic would allow some more tweaks for edge cases.
There was a problem hiding this comment.
I do not think this PR is good to define the approach for "getDependencies". I would love to find the previous discussion, but can't. maybe @kubawerlos or @SpacePossum remember the place?
======
running some fixers multiple times
👎🏻
There was a problem hiding this comment.
I do not think this PR is good to define the approach for "getDependencies"
Probably you're right, I just dump my thoughts about how it should be done properly, because I believe investing time in phpDoc formatting does not make much sense. I actually don't feel the need to have this phpDoc in first place, I never use this information from there. Explicit dependency graph should only require integration tests for fixers, not modifying phpDoc.
Anyway, format proposed by @localheinz is better in terms of git diff (just like multiline arrays or signatures), but has worse DX when you don't collapse phpDoc by default. So I'm pretty +0 about it 🤷♂️.
running some fixers multiple times
👎🏻
As I said, dynamic order resolution would probably fix that "priority" problems anyway, so it was only example of how that resolver would allow us to hook into the final fixers' order. I understand that running fixer(s) multiple times would make the process longer 🙂.
There was a problem hiding this comment.
I've setup'ed the priorities in FixerFactoryTest in preparation for topological sorting, yet the fixed value ones and some exception cases (like the PHPDoc ones), circular dependencies and BC concerns made be stop there.
It was discussed mostly off GH I think, there are some traces of the talks (for example #6155 (comment)), but might be better to start fresh?
There was a problem hiding this comment.
It seems like @kubawerlos had exact same idea as me, with slightly different naming and implementation details (new interface vs AbstractFixer). So it seems like a good direction 🙂.
| @@ -43,7 +43,9 @@ public function getDefinition(): FixerDefinitionInterface | |||
| /** | |||
| * {@inheritdoc} | |||
| * | |||
| * Must run after PhpdocToCommentFixer. | |||
| * Must run after: | |||
| * | |||
localheinz
force-pushed
the
feature/list
branch
from
589bb4f
to
6acbc69
Compare
| @@ -53,7 +53,12 @@ public function getDefinition(): FixerDefinitionInterface | |||
| /** | |||
| * {@inheritdoc} | |||
| * | |||
| * Must run before EscapeImplicitBackslashesFixer, ExplicitStringVariableFixer, NativeFunctionInvocationFixer, SingleQuoteFixer. | |||
There was a problem hiding this comment.
moving back to the original goal of this PR.
@localheinz , for me those comments are having almost no value anymore. They are not enough, we set up different approaches to have the priorities tested, and the comment here with 50 fixers listed is not useful (for me) anyway.
With that, if those comments are not useful for you, I suggest to drop this PR in the end, and we should have better solution (like discussed in other thread) for long run.
Yet, if those comments are bringing any value to you, come up with approach that is the best for you and I will gladly merge it.
There was a problem hiding this comment.
Then let's leave everything as it is or remove the comments entirely, @keradus - what do you think?
I only stumbled across this issue with the list of fixers in a separate pull request, took a note, and figured it would be easier to maintain if it was a vertical list (can easily move a line of code up or down in PhpStorm) rather than a horizontal list.
For me, these comments provide no value at the moment.
There was a problem hiding this comment.
If the list provides no value, there is no value to change format of that list. But it is also work already done here to do so, thus no extra effort.
Then, dropping the list at all would actually bring some new effort, maybe not needed for now.
Please, decide and apply the conclusion.
|
@localheinz , which direction you would like to go ?
|
|
Let's keep it as it is for now! |
This pull request
💁♂️ This could make managing the list a bit easier (you could now slide an item up and down)