-
-
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
feat: Introduce FullyMultilineFixer for ControlStructure, Array & Functions. #7813
base: master
Are you sure you want to change the base?
feat: Introduce FullyMultilineFixer for ControlStructure, Array & Functions. #7813
Conversation
|
||
Allowed values: a subset of ``['arguments', 'arrays', 'case', 'control_structures', 'match', 'parameters']`` | ||
|
||
Default value: ``['arrays']`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the default value:
- Since this rule is kinda similar/related to the TrailingCommaInMultilineFixer, I copied the default value
- But if we're driven by the PSR12, the default value might be something like
control_structure
+parameters
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a new rule, what about enabling everything by default?
I don't think I ever created a rule so far. I'll be happy to get some feedback (for instance about the name or the config) |
a81d71d
to
65ea695
Compare
Great job @VincentLanglet 😄 Looking forward to this PR being merged. Not having this enforced in the PSR12 rule currently causes PHP linters (configured to PSR-12) to fail even after running the fixer on the given file(s). Keep up the good work ❤️ Edit: I know it's a WIP, but from what I tested (and what the tests expect as well), it transforms this: $arr = ['test',
'test2']; Into this: $arr = [
'test',
'test2',
]; Shouldn't it base the fixes indentation based on the line containing the opening token (bracket, brace, etc.)? So the closing token would be aligned with the opening statement and everything inside would get indented once per level: $arr = [
'test',
'test2',
]; Yes the |
Yes, you'll get
I think it's better to rely to the |
f63d1b5
to
6375309
Compare
80e571e
to
44ecdc4
Compare
$a = foo(1, 2, | ||
3 | ||
);', | ||
['elements' => [SingleExpressionPerLineFixer::ELEMENTS_ARGUMENTS]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better not to use such constants here and to use hardcoded values instead. You want your tests to fail if the accepted values change in the implementation, which is not possible when the implementation's constants are also used in the test.
Then maybe the constants could be made private in the implementation.
/** | ||
* @author Vincent Langlet | ||
*/ | ||
final class SingleExpressionPerLineFixer extends AbstractFixer implements ConfigurableFixerInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of this new rule are already implemented in method_argument_space
(with the on_multiline
option). Should we rework that rule somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just discover that there is an overlap with method_argument_space
.
I was wondering if I should remove the argument
option from my Fixer.
if ($tokens[$prevIndex]->equalsAny([']', [T_CLASS], [T_STRING], [T_VARIABLE], [T_STATIC]]) | ||
&& !$tokens[$prevPrevIndex]->isGivenKind(T_FUNCTION) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been fixed automatically 😅
if ($tokens[$prevIndex]->equalsAny([']', [T_CLASS], [T_STRING], [T_VARIABLE], [T_STATIC]]) | |
&& !$tokens[$prevPrevIndex]->isGivenKind(T_FUNCTION) | |
) { | |
if ( | |
$tokens[$prevIndex]->equalsAny([']', [T_CLASS], [T_STRING], [T_VARIABLE], [T_STATIC]]) | |
&& !$tokens[$prevPrevIndex]->isGivenKind(T_FUNCTION) | |
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPCSFixer extends Symfony ruleset which disable the FullyMultilineFixer...because it will do too much change to the symfony codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the rule is wanted by Symfony but temporarily disabled for practical reasons, I'd say it should be enabled in @Symfony
ruleset (so it can be applied to e.g. third party bundles) and disabled in the Symfony repository.
|
||
private function addNewLineAfter(Tokens $tokens, int $index, int $extraIndentation = 0): int | ||
{ | ||
$indent = $this->getIndentation($tokens, $index, $extraIndentation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove indentation handling and leave that to statement_indentation
and array_indentation
rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the indentation handling from MethodArgumentSpaceFixer.
If the method argument is doing it, I was thinking it could be nice do it too.
But I'd like to avoid indeed.
If I want to rely on statement_indentation and array_indentation, I need them to be perfect.
And #7884 is one blocker for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should rather rework method_argument_space
to not handle indentation too (maybe with a deprecation path though).
For the record, you should be able to work around #7884 by fixing the wrong indent manually. The rule should not "unfix" it on subsequent run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, you should be able to work around #7884 by fixing the wrong indent manually. The rule should not "unfix" it on subsequent run.
Yes, but the rule won't be popular (especially for big code base) if you have to fix every single multi-line controle structure indentation after a run... It's definitely a no-go IMHO.
if ($a | ||
|| $b) { | ||
// foo | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test will pass if you write:
if ($a | |
|| $b) { | |
// foo | |
} | |
if ($a | |
|| $b) { | |
// foo | |
} |
This is because currently, statement_indentation
handles $a || $b
as a single statement, so line || $b
is not fixed but simply adjusted with the same indentation delta as line $a
, i.e. 4 spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but since most of the existing code base are using
if ($a
|| $b) {
// foo
}
I think, we must fix this correctly.
That's why I consider #7884 as a blocker.
Imho this PR is ready only after this test (untouched) is finally green.
?array $lineDelimitersIndexes = null | ||
): int { | ||
$tokenAddedCount = 0; | ||
if (!$tokens->isPartialCodeMultiline($begin, $end)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that a structure that does not directly contain newlines but contains another structure that does will be fixed anyway? e.g.:
-$a = [foo(
+$a = [
+foo(
1,
2,
-)];
+)
+];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
In the example
$a = [foo(
1,
2,
)];
the array is multiline by definition because the open [ and the close ] are not on the same line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm wrong but my interpretation of those "fully multiline" rules has always been that we should only consider newlines that are directly part of the structure we are fixing. So in this example, the foo()
function must be fully multiline, but not the array itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, cf https://www.php-fig.org/psr/psr-12/#47-method-and-function-calls
A single argument being split across multiple lines (as might be the case with an anonymous function or array) does not constitute splitting the argument list itself.
so I need to fix this...
Will this supersede #6774 and/or |
@VincentLanglet once again thank you very much @VincentLanglet for your work here and sorry for not being able to tackle this in past weeks (months?), but I really did not have enough time to focus on this as much as it requires. Now, when |
Kinda related to discussion #7813 (comment) There is an overlap between the two rules.
No problem @Wirone, currently I'm waiting for #7901 to be merged |
Closes #6675
Closes #5393
Help #4502