Skip to content

Extract DeclareParenthesesFixer from BracesFixer#5751

Merged
keradus merged 2 commits intoPHP-CS-Fixer:masterfrom
julienfalque:split-braces-declare-braces
Aug 29, 2021
Merged

Extract DeclareParenthesesFixer from BracesFixer#5751
keradus merged 2 commits intoPHP-CS-Fixer:masterfrom
julienfalque:split-braces-declare-braces

Conversation

@julienfalque
Copy link
Copy Markdown
Member

Extracted from #4884.

@coveralls
Copy link
Copy Markdown

coveralls commented May 28, 2021

Coverage Status

Coverage increased (+0.0008%) to 92.317% when pulling d2fd18b on julienfalque:split-braces-declare-braces into 0865c64 on FriendsOfPHP:master.

@julienfalque julienfalque force-pushed the split-braces-declare-braces branch from d16c132 to b973f9f Compare June 3, 2021 17:22
@julienfalque julienfalque changed the base branch from 2.19 to master June 3, 2021 17:23
@kubawerlos kubawerlos mentioned this pull request Jul 20, 2021
2 tasks
public function getDefinition(): FixerDefinitionInterface
{
return new FixerDefinition(
'There must not be spaces around `declare` statement braces.',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

around braces or inside parenthesis ?

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.

around :)

Copy link
Copy Markdown
Member

@keradus keradus Aug 28, 2021

Choose a reason for hiding this comment

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

to my understanding, braces means {/}
in PHP CS Fixer, for ( / ) we were using parentheses.

i will lat myself apply the rename

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to my understanding, braces means {/}
in PHP CS Fixer, for ( / ) we were using parentheses.

if we were that consistent - we have curly braces even in some fixer name

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

Great job. Should we add config to the braces fixer to get it to not proxy the fixer, so we can deprecate it doing that for removal in next major?

@julienfalque
Copy link
Copy Markdown
Member Author

I would not deprecate braces until we have extracted everything into dedicated rules. Everything is done in #4884 and #4885 but we decided to split #4884 into smaller PR to ease merging.

@GrahamCampbell
Copy link
Copy Markdown
Contributor

GrahamCampbell commented Aug 23, 2021

I am not suggesting deprecating the braces fixer. I'm suggesting deprecating the option in the braces fixer that proxies declare_braces.

@julienfalque
Copy link
Copy Markdown
Member Author

Oh ok. Well, since the point is to deprecate the rule entirely, I'm not sure it's worth the hassle.

@julienfalque julienfalque force-pushed the split-braces-declare-braces branch 2 times, most recently from 95e8508 to abda6cd Compare August 27, 2021 18:29
@kubawerlos kubawerlos added the RTM Ready To Merge label Aug 27, 2021
@kubawerlos kubawerlos added this to the 3.1.0 milestone Aug 27, 2021
@keradus keradus force-pushed the split-braces-declare-braces branch from abda6cd to a2f9c23 Compare August 28, 2021 15:02
@keradus keradus changed the title Extract DeclareBracesFixer from BracesFixer Extract DeclareParenthesesFixer from BracesFixer Aug 29, 2021
@keradus keradus removed the RTM Ready To Merge label Aug 29, 2021
@keradus
Copy link
Copy Markdown
Member

keradus commented Aug 29, 2021

Thank you @julienfalque.

@keradus keradus merged commit 3f2a49e into PHP-CS-Fixer:master Aug 29, 2021
@julienfalque julienfalque deleted the split-braces-declare-braces branch August 29, 2021 19:58
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.

5 participants