Priority: AlignMultilineComment should run before every PhpdocFixer #4085

dmvdbrugge · 2018-11-08T15:35:22Z

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

src/Fixer/Basic/BracesFixer.php

keradus · 2018-11-20T23:38:43Z

src/AbstractNoUselessElseFixer.php

@@ -26,7 +26,7 @@
    public function getPriority()
    {
        // should be run before NoWhitespaceInBlankLineFixer, NoExtraBlankLinesFixer, BracesFixer and after NoEmptyStatementFixer.
-        return 25;
+        return 34;


why that change?

Because (as the comment says) it should run before BracesFixer, which has its priority changed

src/Fixer/ControlStructure/ElseifFixer.php

keradus · 2018-11-20T23:38:58Z

src/Fixer/ControlStructure/NoAlternativeSyntaxFixer.php

@@ -67,7 +67,7 @@ public function isCandidate(Tokens $tokens)
     */
    public function getPriority()
    {
-        return 1;
+        return 35;


why that change ?

and for other changes...

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.

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 ?

Unfortunately, no. If it would've, I'd have done that instead 😉

You can see that in the graph as well.

tests/Fixtures/Integration/priority/line_ending,braces.test

keradus · 2018-11-20T23:41:25Z

I see plenty changes of getPriority methods for Fixers that are not Phpdoc*/Braces/LineEnding, without any reasoning added for those changes (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)

dmvdbrugge · 2018-11-20T23:42:13Z

@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.

keradus · 2018-11-20T23:45:34Z

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:

AlignMultilineComment should run before every PhpdocFixer

You added a priority relation between AlignMultilineComment and Phpdoc*Fixers, but you did not added integration tests for that.

For that, I'm not happy to change a priority without adding comment with reasoning and integration test for it.

keradus · 2018-11-20T23:46:57Z

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, LineEnding and Braces can be extracted to separated PR.
Smaller PRs, easier to review & manual QA, easier to spot code changes that shall not be done or are missing tests.

dmvdbrugge

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 and Phpdoc*Fixers, 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...

dmvdbrugge · 2018-11-21T11:15:43Z

src/AbstractNoUselessElseFixer.php

@@ -26,7 +26,7 @@
    public function getPriority()
    {
        // should be run before NoWhitespaceInBlankLineFixer, NoExtraBlankLinesFixer, BracesFixer and after NoEmptyStatementFixer.
-        return 25;
+        return 34;


Because (as the comment says) it should run before BracesFixer, which has its priority changed

src/Fixer/ControlStructure/ElseifFixer.php

dmvdbrugge · 2018-11-21T13:17:23Z

src/Fixer/ControlStructure/NoAlternativeSyntaxFixer.php

@@ -67,7 +67,7 @@ public function isCandidate(Tokens $tokens)
     */
    public function getPriority()
    {
-        return 1;
+        return 35;


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.

src/Fixer/Basic/BracesFixer.php

tests/Fixtures/Integration/priority/line_ending,braces.test

keradus · 2018-11-28T01:09:54Z

Smaller PRs

This really is not that big of a PR...

You say this PR is small in terms of diff size. Indeed, it is.
Yet, I'm saying this PR is huge in terms of complexity, impact and potential risk.

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 it's not trivial to validate if the changes are good or not. "small diff" you say? few lines to read, hard to understand the changes, impact impossible to predict, especially with missing tests.

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.

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).

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.

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.
(...)
Yes, sorry, none of the "special phpdoc cases" have integration tests as far as I could see.

So, if some legacy code has no tests, it's not an excuse to not create tests for new code/scenarios.
I'm not asking you to add in this PR tests for existing-not-covered cases.
I'm asking you to add them for new priority relation cases you are adding.
You added changes in "special cases" - provideFixersPrioritySpecialPhpdocCases https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/56557496da75f1feec1e12377c500f7a9b4688c2/tests/AutoReview/FixerFactoryTest.php#L219 . Those are that legacy cases without proper testing, and those are ignored in our test guard: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/56557496da75f1feec1e12377c500f7a9b4688c2/tests/AutoReview/FixerFactoryTest.php#L310 .
We shall not add more tests that are not covered by the guard.

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.

The comment part, especially after I will boyscout these (kudos :) ), is good part of the changes.
Lack of integration tests for newly configured priority relationship, is not.

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.

I prefer to merge multiple small, easy and safe PRs, one after another, steps by steps, instead of having one problematic, long-living PR.

dmvdbrugge · 2019-01-07T21:53:22Z

I pulled out the LineEnding and Braces part into #4245 together with the added comments.

Also add comments...

This PR was merged into the 2.12 branch. Discussion ---------- LineEndingFixer - BracesFixer - Priority Separated the `line_ending`/`braces`-priority relation and some comments from #4085 Commits ------- 33f5fcd Define priority between LineEndingFixer and BracesFixer

julienfalque · 2019-10-07T20:54:27Z

@dmvdbrugge #4245 was merged, can you please rebase this PR?

dmvdbrugge · 2019-10-10T16:49:15Z

@julienfalque rebase done, had to work in a "new" (since a year ago) fixer, but all seems good again.

dmvdbrugge · 2020-01-24T16:17:01Z

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.

keradus · 2020-04-10T16:45:45Z

a lot of integration tests were already added to cover some of those missing priority cases.
would you mind rebase and see what is still missing to cover with tests, @dmvdbrugge ?
i know it's a lot to ask, no need to rush on it ;)

kubawerlos · 2020-07-13T17:27:04Z

src/Fixer/ClassNotation/SingleTraitInsertPerStatementFixer.php

@@ -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


@dmvdbrugge really? Can you point a fixer in 2.15 that is using should? ;)

keradus · 2021-03-10T20:56:55Z

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.

kubawerlos

This kind of multi-priority changes (which are necessary of course) makes me think that #5078 would save a lot of trouble.

coveralls · 2021-03-10T21:09:26Z

Coverage remained the same at 91.864% when pulling 701708e on dmvdbrugge:3879-AlignMultiline-before-Phpdoc into cbe98ff on FriendsOfPHP:2.18.

keradus · 2021-03-17T09:23:53Z

@kubawerlos , i believe we should aim to drop numbers and have relations, like @SpacePossum described here: #5078 (comment)

keradus · 2021-03-17T18:09:19Z

The failure we see in the test for PHP8 build is caused by not-known-before fixers relation between method_argument_space and function_declaration, and as we were not aware about the relation, we did not have explicit test for it.
Now, we accidentally discovered it with PHP8 integration test.

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.
This time we were lucky that one of the test, even when not designed for it, discovered the problem.

FYI @dmvdbrugge @julienfalque @SpacePossum @kubawerlos

keradus · 2021-03-17T18:29:46Z

Trying to solve above issue in #5552 ,
then we should unblock this PR

…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

keradus · 2021-03-17T20:25:47Z

Thank you @dmvdbrugge.

dmvdbrugge · 2021-03-17T20:35:59Z

Thanks a lot for finishing this @keradus and my apologies for being awol.

This was referenced Nov 8, 2018

(Multiple?) priority issues #3844

Closed

Priority: braces should run before no_trailing_whitespace_in_comment #3880

Closed

BracesFixer - NoTrailingWhitespaceInCommentFixer - Priority #4094

Closed

dmvdbrugge force-pushed the 3879-AlignMultiline-before-Phpdoc branch from 42dd93b to 69003f1 Compare November 13, 2018 09:39

keradus reviewed Nov 20, 2018

View reviewed changes

src/Fixer/Basic/BracesFixer.php Outdated Show resolved Hide resolved

keradus reviewed Nov 20, 2018

View reviewed changes

src/Fixer/ControlStructure/ElseifFixer.php Outdated Show resolved Hide resolved

keradus reviewed Nov 20, 2018

View reviewed changes

tests/Fixtures/Integration/priority/line_ending,braces.test Outdated Show resolved Hide resolved

dmvdbrugge commented Nov 21, 2018

View reviewed changes

keradus self-assigned this Nov 28, 2018

dmvdbrugge mentioned this pull request Jan 7, 2019

LineEndingFixer - BracesFixer - Priority #4245

Merged

dmvdbrugge added a commit to dmvdbrugge/PHP-CS-Fixer that referenced this pull request Jan 7, 2019

Dont wait on PHP-CS-Fixer#4085 just specify priority

5ecb25c

Also add comments...

keradus changed the base branch from 2.12 to 2.15 September 4, 2019 11:01

dmvdbrugge force-pushed the 3879-AlignMultiline-before-Phpdoc branch from 5655749 to 63c3500 Compare October 10, 2019 16:28

julienfalque approved these changes Oct 11, 2019

View reviewed changes

julienfalque added the kind/bug label Oct 11, 2019

julienfalque added this to the 2.15.4 milestone Oct 11, 2019

keradus modified the milestones: 2.15.4, 2.15.5 Nov 2, 2019

keradus removed this from the 2.15.5 milestone Nov 24, 2019

keradus modified the milestones: 2.15.6, 2.15.7 Apr 10, 2020

keradus modified the milestones: 2.15.7, 2.15.8 Apr 15, 2020

keradus modified the milestones: 2.15.8, 2.15.9 Jun 27, 2020

kubawerlos requested changes Jul 13, 2020

View reviewed changes

keradus modified the milestones: 2.15.9, 2.15.10 Oct 27, 2020

keradus removed this from the 2.15.10 milestone Nov 20, 2020

keradus changed the base branch from 2.15 to 2.16 November 20, 2020 13:54

keradus changed the base branch from 2.16 to 2.17 December 17, 2020 13:40

keradus changed the base branch from 2.17 to 2.18 January 24, 2021 21:36

keradus added this to the 2.18.4 milestone Mar 10, 2021

kubawerlos approved these changes Mar 10, 2021

View reviewed changes

keradus mentioned this pull request Mar 17, 2021

DX: test relation between function_declaration and method_argument_space #5552

Merged

Priority: AlignMultilineComment should run before every PhpdocFixer

701708e

keradus force-pushed the 3879-AlignMultiline-before-Phpdoc branch from f49f879 to 701708e Compare March 17, 2021 20:24

keradus merged commit fc4791c into PHP-CS-Fixer:2.18 Mar 17, 2021

dmvdbrugge deleted the 3879-AlignMultiline-before-Phpdoc branch March 17, 2021 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Priority: AlignMultilineComment should run before every PhpdocFixer #4085

Priority: AlignMultilineComment should run before every PhpdocFixer #4085

dmvdbrugge commented Nov 8, 2018 •

edited

keradus Nov 20, 2018

dmvdbrugge Nov 21, 2018

keradus Nov 20, 2018

keradus Nov 20, 2018

dmvdbrugge Nov 21, 2018

keradus Nov 28, 2018

dmvdbrugge Dec 6, 2018

keradus commented Nov 20, 2018

dmvdbrugge commented Nov 20, 2018

keradus commented Nov 20, 2018

keradus commented Nov 20, 2018

dmvdbrugge left a comment

dmvdbrugge Nov 21, 2018

dmvdbrugge Nov 21, 2018

keradus commented Nov 28, 2018

dmvdbrugge commented Jan 7, 2019

julienfalque commented Oct 7, 2019

dmvdbrugge commented Oct 10, 2019

dmvdbrugge commented Jan 24, 2020

keradus commented Apr 10, 2020

kubawerlos Jul 13, 2020

keradus commented Mar 10, 2021

kubawerlos left a comment

coveralls commented Mar 10, 2021 •

edited

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

dmvdbrugge commented Mar 17, 2021

Priority: AlignMultilineComment should run before every PhpdocFixer #4085

Priority: AlignMultilineComment should run before every PhpdocFixer #4085

Conversation

dmvdbrugge commented Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Nov 20, 2018

dmvdbrugge commented Nov 20, 2018

keradus commented Nov 20, 2018

keradus commented Nov 20, 2018

dmvdbrugge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Nov 28, 2018

dmvdbrugge commented Jan 7, 2019

julienfalque commented Oct 7, 2019

dmvdbrugge commented Oct 10, 2019

dmvdbrugge commented Jan 24, 2020

keradus commented Apr 10, 2020

Choose a reason for hiding this comment

keradus commented Mar 10, 2021

kubawerlos left a comment

Choose a reason for hiding this comment

coveralls commented Mar 10, 2021 • edited

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

keradus commented Mar 17, 2021

dmvdbrugge commented Mar 17, 2021

dmvdbrugge commented Nov 8, 2018 •

edited

coveralls commented Mar 10, 2021 •

edited