-
-
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
Priority: AlignMultilineComment should run before every PhpdocFixer #4085
Priority: AlignMultilineComment should run before every PhpdocFixer #4085
Conversation
42dd93b
to
69003f1
Compare
src/AbstractNoUselessElseFixer.php
Outdated
@@ -26,7 +26,7 @@ | |||
public function getPriority() | |||
{ | |||
// should be run before NoWhitespaceInBlankLineFixer, NoExtraBlankLinesFixer, BracesFixer and after NoEmptyStatementFixer. | |||
return 25; | |||
return 34; |
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.
why that change?
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.
Because (as the comment says) it should run before BracesFixer
, which has its priority changed
@@ -67,7 +67,7 @@ public function isCandidate(Tokens $tokens) | |||
*/ | |||
public function getPriority() | |||
{ | |||
return 1; | |||
return 35; |
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.
why that change ?
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.
and for other changes...
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.
why that change ?
Because it needs to run before ElseifFixer
, which has its priority changed. I will add the comment here too.
and for other changes...
Again, please look at the graph. AlignMultilineCommentFixer
needed to change from -40 to +30. All other changes follow from that. It's not a process of trial and error, it's calculated using the graph. All defined priority-relations are still adhered to.
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.
AlignMultilineComment should run before every PhpdocFixer
AlignMultilineCommentFixer
needed to change from -40 to +30
would adjusting priority of PhpdocFixer
instead of AlignMultilineCommentFixer
produce less priority changes ?
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.
Unfortunately, no. If it would've, I'd have done that instead 😉
You can see that in the graph as well.
I see plenty changes of |
@keradus the answer to all your "why" is because of all the existing priorities. See the graph I mentioned, see all defined priorities, and see that all tests still pass. |
half of priorities are not covered with i-tests, eg even the one you are just saying you are fixing in this PR:
You added a priority relation between For that, I'm not happy to change a priority without adding comment with reasoning and integration test for it. |
Priority is one of the most tricky part of this tool. Maybe let's try to split this PR into atomic pieces, eg for what I understand, |
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.
I see plenty changes of getPriority methods for Fixers that are not
Phpdoc*/Braces/LineEnding
, without any reasoning added for those changes
As I mentioned before, it's a cascading change (again, see the graph, which visualises all current defined relations).
(keep in mind that when we do non-0 prio, we put a comment why - and those comments were either not added in your PR, or not updated while update of prio)
Those comments should imho not be on "non-0 prio" but any time a priority-relation is defined, whether or not one of them is 0 (but that's maybe a different discussion). So they should've already existed in most of the cases. However like I commented inline, I will boyscout these.
half of priorities are not covered with i-tests
So we should write them. Please don't let that stop existing PRs that try to fix priority-problems.
eg even the one you are just saying you are fixing in this PR:
AlignMultilineComment should run before every PhpdocFixer
You added a priority relation between
AlignMultilineComment
andPhpdoc*Fixer
s, but you did not added integration tests for that.
Yes, sorry, none of the "special phpdoc cases" have integration tests as far as I could see.
For that, I'm not happy to change a priority without adding comment with reasoning and integration test for it.
The comment is there though. I could link to the issue as well. The reasoning I hope is clear to you: It fixes the *
s of docblocks, they could be missing. And while that still is a valid docblock, there are fixers that won't run (correctly) in that case.
Priority is one of the most tricky part of this tool.
Which is exactly why I made the graph before even attempting this PR.
Maybe let's try to split this PR into atomic pieces, eg for what I understand,
LineEnding and Braces
can be extracted to separated PR.
In theory yes, however that would block the current PR which needs to change those priorities, that's why it was found in the first place, and needed to be fixed as well.
Smaller PRs
This really is not that big of a PR...
src/AbstractNoUselessElseFixer.php
Outdated
@@ -26,7 +26,7 @@ | |||
public function getPriority() | |||
{ | |||
// should be run before NoWhitespaceInBlankLineFixer, NoExtraBlankLinesFixer, BracesFixer and after NoEmptyStatementFixer. | |||
return 25; | |||
return 34; |
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.
Because (as the comment says) it should run before BracesFixer
, which has its priority changed
@@ -67,7 +67,7 @@ public function isCandidate(Tokens $tokens) | |||
*/ | |||
public function getPriority() | |||
{ | |||
return 1; | |||
return 35; |
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.
why that change ?
Because it needs to run before ElseifFixer
, which has its priority changed. I will add the comment here too.
and for other changes...
Again, please look at the graph. AlignMultilineCommentFixer
needed to change from -40 to +30. All other changes follow from that. It's not a process of trial and error, it's calculated using the graph. All defined priority-relations are still adhered to.
You say this PR is small in terms of diff size. Indeed, it is. Don't get me wrong - your work here is super nice, simply part of the code you are touching is super fragile, and any mistake here has big consequences. And well, outcome of it is clear, nobody wants to review it (don't worry, I don't want to sound harsh here, this PR is on my watchlist and won't get forgotten for sure (even if I can't spend as much time with it as I would like to)), as nobody can say "it's safe to release this PR" - I'm trying to ask you for changes, that would make us more safe to release it.
and for that, as it's all-in-one, hard to see out of the box which change in code is related to what change we want to accomplish.
So, if some legacy code has no tests, it's not an excuse to not create tests for new code/scenarios.
The comment part, especially after
I prefer to merge multiple small, easy and safe PRs, one after another, steps by steps, instead of having one problematic, long-living PR. |
I pulled out the |
Also add comments...
@dmvdbrugge #4245 was merged, can you please rebase this PR? |
5655749
to
63c3500
Compare
@julienfalque rebase done, had to work in a "new" (since a year ago) fixer, but all seems good again. |
It might be a lot to ask, but please read the entire discussion in this PR again, because all is explained already. TLDR: the required change is the title (AlignMultilineComment before the Phpdoc*), all other changes is to keep priority correct. See the graphs. |
a lot of integration tests were already added to cover some of those missing priority cases. |
@@ -43,8 +43,8 @@ final class Example | |||
|
|||
public function getPriority() | |||
{ | |||
// must be run before Braces and SpaceAfterSemicolonFixer | |||
return 1; | |||
// should be run before Braces, SpaceAfterSemicolonFixer |
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.
@dmvdbrugge really? Can you point a fixer in 2.15
that is using should? ;)
I let myself integrate the integration test into the PR, with having the actual test proving the changes are solving the issue, I'm feeling much more comfortable merging this. I synced the branch (ignore the merge commit and fabbot complain, I gonna squash it during merging anyway) and put milestone back. |
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.
This kind of multi-priority changes (which are necessary of course) makes me think that #5078 would save a lot of trouble.
@kubawerlos , i believe we should aim to drop numbers and have relations, like @SpacePossum described here: #5078 (comment) |
The failure we see in the test for PHP8 build is caused by not-known-before fixers relation between Having all relations discovered and documented with dedicated test is almost impossible, and changing the priorities like in this PR is putting us at risk to introduce such issues. |
Trying to solve above issue in #5552 , |
…rgument_space (keradus) This PR was merged into the 2.18 branch. Discussion ---------- DX: test relation between function_declaration and method_argument_space found here: #4085 (comment) Commits ------- ca36da5 DX: test relation between function_declaration and method_argument_space
f49f879
to
701708e
Compare
Thank you @dmvdbrugge. |
Thanks a lot for finishing this @keradus and my apologies for being awol. |
I made use of the graph I generated with https://github.com/dmvdbrugge/fixer-prio-graph
I also found a hidden priority between LineEnding and Braces, which I had to add in this PR as well, because otherwise the
@Symfony_whitespace.test
would fail.Fixes #3879