-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Add initial in-statements indent support #7901
base: master
Are you sure you want to change the base?
fix: Add initial in-statements indent support #7901
Conversation
b500122
to
a569c25
Compare
Thanks for this PR,
I'm not sure to understand the behavior or the reason behind this behavior. If I understand correctly,
are not fixed the same way, same for
? Could elaborate the reason/need of this choice ? :) |
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 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 ? |
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. |
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. |
I agree. i did some tests and just found that
is fixed to
do we consider this as ok or should it be
? |
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. |
should not be that hard |
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) {} ```
a569c25
to
8a0a25e
Compare
Is it possible to move this forward ? |
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 😊. |
If it would help I follow this repo and can review some PRs on code/areas I worked on. |
Every review is helpful, especially from people already familiar with the codebase 😊. Feel free to do it if you have time and will. |
Fixes #7884.
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.
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.
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:
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.