fix: Add initial in-statements indent support #7901

julienfalque · 2024-03-20T19:14:30Z

⚠️ This is a big change for statement_indentation: previously, statements (which here means anything that isn't a block or an array) were not fixed but adjusted as a whole, meaning that instead of forcing the indentation level on each line of a given statement, only the first line was fixed, and subsequent lines were changed with the same delta.

For example, in the following snippet, line 3 is indented with one more space than line 2. Line 2 was fixed by adding two spaces, so line 3 was adjusted with 2 more spaces as well.

 if (
-  true
-   || false) {}
+    true
+     || false) {}

This behavior was intended because actually fixing statements that span multiple lines was quite difficult. However, this leads to unexpected results, so I would consider this a bugfix rather than an enhancement.

⚠️ These changes might also introduce a lot of new bugs because there are likely a lot of possible situations and only very few covered in tests. I didn't try running the rule on other projects for now.

Also, currently, new indentation behavior might be more opinionated than before: it adds an extra level of indentation in multiline statements unless they are in a control structure condition or wrapped in parentheses:

 if (
-$foo
-.$bar
+    $foo
+    .$bar
 ) {}

 $a = (
-$foo
-.$bar
+    $foo
+    .$bar
 );

 foo(
-$foo
-.$bar,
-true,
+    $foo
+        .$bar,
+    true,
 );

We might want to make all these changes opt-in with an option, which would prevent maybe unwanted changes in users codebases but few people might notice the new option and enable it.

coveralls · 2024-03-20T19:20:43Z

coverage: 96.044% (+0.009%) from 96.035%
when pulling 8a0a25e on julienfalque:inner-statement-indent
into e980ab2 on PHP-CS-Fixer:master.

VincentLanglet · 2024-03-20T20:09:40Z

Thanks for this PR,

Also, currently, new indentation behavior might be more opinionated than before: it adds an extra level of indentation in multiline statements unless they are in a control structure condition or wrapped in parentheses:
 if (
-$foo
-.$bar
+    $foo
+    .$bar
 ) {}

 $a = (
-$foo
-.$bar
+    $foo
+    .$bar
 );

 foo(
-$foo
-.$bar,
-true,
+    $foo
+        .$bar,
+    true,
 );

I'm not sure to understand the behavior or the reason behind this behavior.
I would have expected the same behavior for the three example.

If I understand correctly,

$a =
$foo
.$bar;

$b = (
$foo
.$bar
);

are not fixed the same way, same for

$a = $this->call(
$foo
.$bar
);

$b = $this->call(
$foo
.$bar,
true
);

?

Could elaborate the reason/need of this choice ? :)

julienfalque · 2024-03-20T22:03:35Z

In control structure conditions you can only have one expression, so if it spans multiple lines, one level of indentation is enough. Same for expressions wrapped in parentheses.

But for function signatures and function calls, you might have multiple expressions (multiple parameters or arguments), so adding one extra level of indentation helps making a distinction between newlines for a same argument and newlines for another argument. There are a few examples in the changes in this PR, e.g. in FinalClassFixer.

I'm aware this is quite opinionated, maybe it makes sense to introduce configuration for this.

VincentLanglet · 2024-03-21T10:45:14Z

I'm aware this is quite opinionated, maybe it makes sense to introduce configuration for this.

Does the rule without the opinionated extra-indentation wouldn't give less changes (like avoiding https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7901/files#diff-6a31ac00beb4f67a4c70fdb0d125a15f25c5b6fcb35516bfa926675325c48046R39).

Or is the opinionated-indent needed for some cases in order to fix the bug ?

julienfalque · 2024-03-21T11:02:28Z

It's not strictly needed. It's mostly a side effect of how I fixed the bug that I happen to be OK with. I'm not sure but I think not applying it might require even more changes and thus be harder to implement though.

Wirone · 2024-03-21T12:12:17Z

In short: I like the idea. Just scrolled through diff and fixed code is better IMHO, this is something I would expect it to be. I still need to review tests and actual changes in fixer, though.

VincentLanglet · 2024-03-21T12:53:26Z

fixed code is better IMHO

I agree.

i did some tests and just found that

foo($foo
.$bar,
$foo2
.$bar2);

is fixed to

foo($foo
    .$bar,
    $foo2
        .$bar2);

do we consider this as ok or should it be

foo($foo
        .$bar,
    $foo2
        .$bar2);

?

julienfalque · 2024-03-21T16:55:16Z

IMO it should be fixed to

foo($foo
        .$bar,
    $foo2
        .$bar2);

but I'm pretty sure this will not be very easy to achieve. Fixing it to

foo($foo
    .$bar,
    $foo2
        .$bar2);

looks OK-ish to me so I'd keep this fix for another iteration.

mvorisek · 2024-03-22T08:04:09Z

foo($foo
        .$bar,
    $foo2
        .$bar2);

should not be that hard

julienfalque · 2024-03-23T10:43:16Z

should not be that hard

@mvorisek What about trying yourself then?

What I mean is that this specific case shouldn't be a blocker for this PR, especially since #7813 depends on it.

VincentLanglet · 2024-03-23T11:10:20Z

What I mean is that this specific case shouldn't be a blocker for this PR, especially since #7813 depends on it.

100% agree

Previously, statements (which here means anything that isn't a block or an array) were not fixed but adjusted as a whole, meaning that instead of forcing the indentation level on each line of a given statement, only the first line was fixed, and subsequent lines were changed with the same delta. For example, in the following snippet, line 3 is indented with one more space than line 2. Line 2 was fixed by adding two spaces, so line 3 was adjusted with 2 more spaces as well. ```diff if ( - true - || false) {} + true + || false) {} ```

VincentLanglet · 2024-04-10T17:31:45Z

Is it possible to move this forward ?
I'd like to finish #7813 :)

Wirone · 2024-04-10T17:39:02Z

FYI: I have a busy time again, and can't focus on Fixer as much as I would like to. I am up to date with issues and PRs, I take actions when it does not require much time or can unblock further works, but I just can't do more now. I'll do my best to process this, but can't tell when. Fortunately @kubawerlos is again in the core team, so his approval also can unlock the merge here 😊.

mvorisek · 2024-04-10T17:42:43Z

If it would help I follow this repo and can review some PRs on code/areas I worked on.

Wirone · 2024-04-10T17:49:58Z

Every review is helpful, especially from people already familiar with the codebase 😊. Feel free to do it if you have time and will.

julienfalque added the kind/bug label Mar 20, 2024

julienfalque force-pushed the inner-statement-indent branch 2 times, most recently from b500122 to a569c25 Compare March 20, 2024 19:32

julienfalque force-pushed the inner-statement-indent branch from a569c25 to 8a0a25e Compare March 23, 2024 18:19

julienfalque changed the title ~~feat: Add initial in-statements indent support~~ fix: Add initial in-statements indent support Mar 23, 2024

keradus force-pushed the master branch from a968b6e to 28209a2 Compare April 17, 2024 08:10

VincentLanglet mentioned this pull request Apr 26, 2024

Enforce new line for each array item if there's already at least one new line #6675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add initial in-statements indent support #7901

fix: Add initial in-statements indent support #7901

julienfalque commented Mar 20, 2024

coveralls commented Mar 20, 2024 •

edited

VincentLanglet commented Mar 20, 2024 •

edited

julienfalque commented Mar 20, 2024

VincentLanglet commented Mar 21, 2024

julienfalque commented Mar 21, 2024

Wirone commented Mar 21, 2024

VincentLanglet commented Mar 21, 2024

julienfalque commented Mar 21, 2024

mvorisek commented Mar 22, 2024

julienfalque commented Mar 23, 2024

VincentLanglet commented Mar 23, 2024

VincentLanglet commented Apr 10, 2024

Wirone commented Apr 10, 2024

mvorisek commented Apr 10, 2024

Wirone commented Apr 10, 2024

fix: Add initial in-statements indent support #7901

Are you sure you want to change the base?

fix: Add initial in-statements indent support #7901

Conversation

julienfalque commented Mar 20, 2024

coveralls commented Mar 20, 2024 • edited

VincentLanglet commented Mar 20, 2024 • edited

julienfalque commented Mar 20, 2024

VincentLanglet commented Mar 21, 2024

julienfalque commented Mar 21, 2024

Wirone commented Mar 21, 2024

VincentLanglet commented Mar 21, 2024

julienfalque commented Mar 21, 2024

mvorisek commented Mar 22, 2024

julienfalque commented Mar 23, 2024

VincentLanglet commented Mar 23, 2024

VincentLanglet commented Apr 10, 2024

Wirone commented Apr 10, 2024

mvorisek commented Apr 10, 2024

Wirone commented Apr 10, 2024

coveralls commented Mar 20, 2024 •

edited

VincentLanglet commented Mar 20, 2024 •

edited