Skip to content

minor: Split BracesFixer#4884

Merged
keradus merged 1 commit intoPHP-CS-Fixer:masterfrom
julienfalque:split-braces-fixer-2.15
Aug 17, 2022
Merged

minor: Split BracesFixer#4884
keradus merged 1 commit intoPHP-CS-Fixer:masterfrom
julienfalque:split-braces-fixer-2.15

Conversation

@julienfalque
Copy link
Copy Markdown
Member

@julienfalque julienfalque commented Mar 16, 2020

@julienfalque julienfalque mentioned this pull request Mar 16, 2020
2 tasks
@julienfalque julienfalque force-pushed the split-braces-fixer-2.15 branch 7 times, most recently from 9202f6e to 7bcb815 Compare March 18, 2020 08:06
@julienfalque julienfalque linked an issue Mar 18, 2020 that may be closed by this pull request
@wshaver
Copy link
Copy Markdown

wshaver commented Oct 12, 2020

Hoping for progress on this for comments and php open tag issues. Details here:
#5019 (comment)

@keradus keradus changed the base branch from 2.15 to 2.16 November 20, 2020 15:24
@keradus keradus changed the base branch from 2.16 to 2.17 December 17, 2020 13:42
@julienfalque julienfalque force-pushed the split-braces-fixer-2.15 branch from 2013401 to a74eeea Compare July 21, 2022 19:05
@julienfalque julienfalque force-pushed the split-braces-fixer-2.15 branch 3 times, most recently from 9c336d3 to 456f1af Compare August 4, 2022 15:31
@julienfalque julienfalque changed the title Split BracesFixer minor: Split BracesFixer Aug 6, 2022
@julienfalque julienfalque marked this pull request as ready for review August 6, 2022 15:14
@julienfalque
Copy link
Copy Markdown
Member Author

julienfalque commented Aug 6, 2022

This PR is now small enough to be reviewed directly IMO.

continue;
}

$pos = strrpos($content, "\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a clean up or can we maybe add a utest for this change to NoExtraBlankLinesFixerTest as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only a refactoring that shouldn't change behavior.

Copy link
Copy Markdown
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So big effort, in this and extracted PRs!
big, big kudos, @julienfalque !

@keradus keradus merged commit c1bcf0c into PHP-CS-Fixer:master Aug 17, 2022
@TomasVotruba
Copy link
Copy Markdown

TomasVotruba commented Aug 18, 2022

Thanks for the split 👍 It will be helfpul in narrowing down changes and bugs!

It seems this split broke indent of catch/switch. I tried the latest 3.9.x and these worked.
How should I setup the rules to keep the original behavior?

-switch ($simple) {\n
+switch($simple) {\n
-} catch (FirstThrowableType $e) {\n
+} catch(FirstThrowableType $e) {\n
 new class($a, $b, $c) extends SomeExtendedClass implements\n
     \ArrayAccess,\n
     \Countable {\n
-        // Body\n
-    };\n
+    // Body\n
+};\n

@TomasVotruba
Copy link
Copy Markdown

Allright, add SingleSpaceAfterConstructFixer solves first 2 cases 👍 🙂

@julienfalque julienfalque deleted the split-braces-fixer-2.15 branch August 18, 2022 20:59
@julienfalque
Copy link
Copy Markdown
Member Author

@TomasVotruba Third case looks expected though.

@TomasVotruba
Copy link
Copy Markdown

TomasVotruba commented Aug 18, 2022

@julienfalque I see people complaining PSR12 set is now broken :) How can I make it 3.9 compatible?
Or is it a bugfix?

@julienfalque
Copy link
Copy Markdown
Member Author

@TomasVotruba Please submit an issue with details regarding what is broken.

@kayw-geek
Copy link
Copy Markdown
Contributor

@julienfalque
I found a bug in the following:

<?php
    class Foo
    {
    public function test()
    { 
    }
    }

it will be fixed before v3.10.0, but it doesn't fix now.

This PR is big change, so I not found where line has a problem

@kayw-geek
Copy link
Copy Markdown
Contributor

@julienfalque I found a bug in the following:

<?php
    class Foo
    {
    public function test()
    { 
    }
    }

it will be fixed before v3.10.0, but it doesn't fix now.

This PR is big change, so I not found where line has a problem

src/Fixer/Basic/CurlyBracesPositionFixer.php

@keradus
Copy link
Copy Markdown
Member

keradus commented Sep 19, 2022

@kayw-geek , would you mind to open new issue and link this PR? not so many ppl are watching the closed issues/PRs

@XBrayanX
Copy link
Copy Markdown

First of all, thank you very much for your great work.
My Question is, with the new changes, is there a rule to keep empty spaces inside control structures? It is mentioned in #4487

@julienfalque
Copy link
Copy Markdown
Member Author

No, the rules created during this refactoring only cover existing behaviour of braces.

@XBrayanX
Copy link
Copy Markdown

But if you will pay attention to comment #4487? This comment is already 3 years old, please, this functionality is necessary to improve the code's readability.

@keradus
Copy link
Copy Markdown
Member

keradus commented Jan 17, 2023

I don't believe we ever provided this feature, and it's not related to split of Braces fixer. if you would like to provide such feature, please show it's needed by given community [eg some big open source project/framework] and propose a PR for it, thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BracesFixer has too many responsibilities

10 participants