Skip to content
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

NoTrailingCommaInSinglelineFunctionCallFixer - Introduction #6304

Merged
merged 1 commit into from Feb 21, 2022
Merged

NoTrailingCommaInSinglelineFunctionCallFixer - Introduction #6304

merged 1 commit into from Feb 21, 2022

Conversation

SpacePossum
Copy link
Contributor

closes #6141

@SpacePossum SpacePossum marked this pull request as draft February 21, 2022 11:07
@coveralls
Copy link

coveralls commented Feb 21, 2022

Coverage Status

Coverage increased (+0.02%) to 93.126% when pulling cb4c02a on SpacePossum:master_NoTrailingCommaInSinglelineFunctionCallFixer into 0b96918 on FriendsOfPHP:master.

@SpacePossum SpacePossum marked this pull request as ready for review February 21, 2022 13:42
@SpacePossum SpacePossum merged commit 7df9f15 into PHP-CS-Fixer:master Feb 21, 2022
@SpacePossum SpacePossum deleted the master_NoTrailingCommaInSinglelineFunctionCallFixer branch February 21, 2022 15:39
@kubawerlos
Copy link
Contributor

Now we have 4 places we remove trailing comma in single line stuff:

  • fixer no_trailing_comma_in_list_call
  • fixer no_trailing_comma_in_singleline_array
  • fixer no_trailing_comma_in_singleline_function_call
  • option trailing_comma_single_line in fixer function_declaration

Can we go instead with one fixer like there is trailing_comma_in_multiline?

@SpacePossum
Copy link
Contributor Author

SpacePossum commented Feb 21, 2022

We could if someone wants to raise the PR and do all the paperwork around deprecations and such. For me it is 50/50, it makes sense to have it done by for example function_declaration, but also by a dedicated rule, I see both sides so I went with this.

private function clearCommaIfNeeded(Tokens $tokens, int $startIndex, int $endIndex, int $commaIndex): void
{
if (!$tokens->isPartialCodeMultiline($startIndex, $endIndex)) {
$tokens->clearTokenAndMergeSurroundingWhitespace($commaIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this way, not:

            $tokens->removeTrailingWhitespace($beforeEndIndex);
            $tokens->clearAt($beforeEndIndex);

as it's done in NoTrailingCommaInSinglelineArrayFixer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I follow, what would be the benefit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency, better fixing - now code:

$a = [1, 2, 3, ];
foo(1, 2, 3, );

would be changed to:

$a = [1, 2, 3];
foo(1, 2, 3 );

I cannot imagine anyone want to have the comma removed and keep it's trailing whitespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check, I didn't even know removeTrailingWhitespace existed, I think your case is covered through single-responsibility of the rule combined with priorities, but if there is more room for improvement than lets PR it.

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.

no_trailing_comma_in_singleline for method calls and argument list
3 participants