Extract StatementIndentationFixer from BracesFixer by julienfalque · Pull Request #5960 · PHP-CS-Fixer/PHP-CS-Fixer

julienfalque · 2021-09-05T14:26:24Z

Extracted from #4884.

coveralls · 2021-09-05T14:42:29Z

Coverage decreased (-0.2%) to 92.933% when pulling f65b48e on julienfalque:split-braces-fixer-statement-indentation into b0a9fca on FriendsOfPHP:master.

julienfalque · 2021-09-05T14:59:48Z

By the way this one is heavily inspired by ArrayIndentationFixer, which I think received some fixes/improvements. I have to check if they have to be applied to StatementIndentationFixer as well.

keradus · 2021-09-05T18:55:37Z

src/Fixer/Whitespace/StatementIndentationFixer.php

+{
+    private $bracesFixerCompatibility;
+
+    public function __construct(bool $bracesFixerCompatibility = false)


It's a first place where we "configure" fixer via constructor parameter. I don't think it's good.
Also, the name doesn't say what behaviour it actually control - can you describe (in code, as variable name?) what is the behaviour is?

Also, why not pass it as fixer configuration?

I don't think it's good.

Why?

Also, the name doesn't say what behaviour it actually control - can you describe (in code, as variable name?) what is the behaviour is?

statement_indentation has 3 specific cases where the behavior changes depending on this parameter. Would you like to have e.g. 1 parameter for each?

Also, why not pass it as fixer configuration?

Because I don't like the braces specific behavior and I don't want it to remain once braces is removed. This means that the $bracesFixerCompatibility will be dropped along with BracesFixer. We would not be able to do that with a configuration exposed to end users.

Anyway, let's see the outcome of #5960 (comment) before acting here :)

keradus · 2021-09-05T19:01:06Z

src/Fixer/Whitespace/StatementIndentationFixer.php

+    {
+        parent::__construct();
+
+        $this->bracesFixerCompatibility = $bracesFixerCompatibility;


i hardcoded false as value and run tests for BracesFixer. I actually like the changes applied with StatementIndentationFixer. Maybe we should drop this "compatibility" mode at all and apply changes introduces by this Fixer to BracesFixer as bugfixes?

The new behavior when using false is described in #4884 (comment). I'm completely ok to drop the "braces compatibility" mode entirely but that might be considered a BC break as braces behavior would change. Is everyone ok with that?

I don't know where to discuss it, so I will put it in this PR

In statement_indentation: when a comment looks like commented-out code, don't indent it

OK, I agree it could make value for folks who are committing commented code or who are commenting out code and run the fixer run while that. Maybe we should keep behaviour of BracesFixer (for both fixers, old and new, without config)

In statement_indentation: when a single line comment is part of a multiline message, don't indent it

sounds legit, right? let's keep behaviour of BracesFixer (for both fixers, old and new, without config)

In statement_indentation: assume code that is not inside any curly braces block is correctly indented

that was actually all I saw when I applied rule with hardcoded false (probably due to amount of cases when it was applied, compared to points 3 and 4).

it was not assuming that. it was mine "simplification" when I was creating BracesFixer, as somehow I was struggling to apply the logic to code on root level (not nested in curly braces block). If both fixers would now be able to handle root-level code, it's great (for both fixers, old and new, without config)

OK, I agree it could make value for folks who are committing commented code or who are commenting out code and run the fixer run while that. Maybe we should keep behaviour of BracesFixer (for both fixers, old and new, without config)

Since version control systems are widespread, I consider commenting out code useless and a bad practice. We should not support that anymore IMO. Having those comments indented can even help spotting commented-out code that should be removed.

sounds legit, right? let's keep behaviour of BracesFixer (for both fixers, old and new, without config)

Meh. I never saw such case, I assume it's very rare.

So, we both want to drop 5, but you want to keep 3 and 4 while I don't. Any other opinion? :)

3

agree about commiting it, but I can imagine running the Fixer during development, eg automatically by CI on change, and having the code pushed few tabs to right can be distracting

4

I saw it few times, IIRC we added it because someone was raising this topic

(happy to hear more opinions as well)

In statement_indentation: when a comment looks like commented-out code, don't indent it

Unless we're talking about docblocks, I don't think the behaviour of fixers should be influenced by comments' content. Also, when temporarily commented out code is uncommented, it can be easily re-indented by running the fixer again.

In statement_indentation: when a single line comment is part of a multiline message, don't indent it

I usually find comments placed on the same lines with code harder to read, particularly longer ones. When a comment is multiline, I think it should be on a separate line anyway. I've also never seen a case like the one in #4884 (comment) and don't think it needs to be supported.

I don't think we need to analyze comments to see if these (appear to be) commented out code and act differently, so indent like anything else

would be nice to have, but not a hard requirement from me

shouldn't all code outside braces block be un-indented by this fixer?

shouldn't all code outside braces block be un-indented by this fixer?

@SpacePossum Yes, that's what the new statement_indentation rule does, unless it's ran via braces to keep its current behavior. Should I understand that you are in favor of applying this new behavior to braces? If so, looking at the discussion, the consensus seems to be:

indent comments even if they look like commented-out code (dropping specific behavior of braces);

keep indenting single line comments that form multiline messages (importing specific behavior from braces into statement_indentation);

always indent code outside braces (dropping specific behavior of braces).

Which means I could remove the $bracesFixerCompatibility parameter entirely. Is everyone ok with this?

SpacePossum · 2021-10-10T18:43:02Z

src/Tokenizer/Analyzer/AlternativeSyntaxAnalyzer.php

+        T_SWITCH => [T_ENDSWITCH],
+    ];
+
+    public function findAlternativeSyntaxBlockEnd(Tokens $tokens, int $index): int


Will this always return the last of block, like with if elseif endif will it return the index of endif or of elseif?

Can we add tests for nested constructs, something like;

<?php switch(foo()): case 1: switch (foo2()): case 2: if (bar()) { } switch (foo2()): case 4: { switch (foo3()) { // not alternative case 4: { } } } endswitch; endswitch; case 2: switch (foo5()) { // not alternative case 4: echo 1; } endswitch;

with all starts and ends tested?

there might be something reusable/extractable from src/Tokenizer/Analyzer/ControlCaseStructuresAnalyzer.php, not 100% sure it is worth it though

Test added.

Will this always return the last of block, like with if elseif endif will it return the index of endif or of elseif?

If case of if ... elseif ... endif, the end token for if is elseif.

there might be something reusable/extractable from src/Tokenizer/Analyzer/ControlCaseStructuresAnalyzer.php

What could be reused exactly?

src/Fixer/Whitespace/StatementIndentationFixer.php

jrmajor

Seems to break all match expressions.

  match ($fit) {
      'square' => $image->fit(Manipulations::FIT_CROP, $targetSize),
      'height' => $image->height($targetSize),
      default => throw new InvalidArgumentException(),
- };
+     };

SpacePossum · 2021-10-19T11:18:51Z

Maybe we can have a test for enum as well

julienfalque · 2022-01-23T15:52:11Z

Rebased:

test cases with enum added;
bug with match expression fixed;
"current line indent" detection refactored;

keradus · 2022-05-09T19:18:58Z

Thank you @julienfalque.

julienfalque added the kind/refactoring label Sep 5, 2021

julienfalque mentioned this pull request Sep 5, 2021

minor: Split BracesFixer #4884

Merged

6 tasks

keradus reviewed Sep 5, 2021

View reviewed changes

SpacePossum reviewed Oct 10, 2021

View reviewed changes

src/Fixer/Whitespace/StatementIndentationFixer.php Outdated Show resolved Hide resolved

jrmajor reviewed Oct 14, 2021

View reviewed changes

julienfalque force-pushed the split-braces-fixer-statement-indentation branch 5 times, most recently from 3431aa5 to 3918ff9 Compare January 23, 2022 15:45

Extract StatementIndentationFixer from BracesFixer

f65b48e

julienfalque force-pushed the split-braces-fixer-statement-indentation branch from 3918ff9 to f65b48e Compare January 23, 2022 16:38

julienfalque mentioned this pull request Feb 22, 2022

Indentation of the entire file fails. #6307

Closed

keradus self-assigned this Feb 22, 2022

keradus merged commit cc8c0fe into PHP-CS-Fixer:master May 9, 2022

julienfalque deleted the split-braces-fixer-statement-indentation branch May 10, 2022 16:04

GrahamCampbell mentioned this pull request May 15, 2022

Broken regression testing on laravel/framework #6394

Closed

julienfalque mentioned this pull request Aug 14, 2022

BracesFixerTest - test indentation where code and comment in this and previous line #3394

Closed

Wirone mentioned this pull request Mar 15, 2023

indentation problem where code and comment in this and previous line #3395

Closed

HLeithner mentioned this pull request Jul 25, 2023

statement_indentation applies odd indention on switch-case statements #7179

Closed

Uh oh!

Conversation

julienfalque commented Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienfalque commented Sep 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrmajor left a comment

Choose a reason for hiding this comment

Uh oh!

SpacePossum commented Oct 19, 2021

Uh oh!

julienfalque commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keradus commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

julienfalque commented Sep 5, 2021 •

edited

Loading

coveralls commented Sep 5, 2021 •

edited

Loading

julienfalque commented Jan 23, 2022 •

edited

Loading