🌐 AI搜索 & 代理 主页
Skip to content

minor: TrailingCommaInMultilineFixer - Add comma to multiline new static #6380

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.

Sign up for GitHub

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

Jump to bottom
Merged
julienfalque merged 3 commits into from
Aug 13, 2022
Merged

minor: TrailingCommaInMultilineFixer - Add comma to multiline new static #6380

julienfalque merged 3 commits into from
Aug 13, 2022

Conversation

@dpi
Copy link
Contributor

@dpi dpi commented May 11, 2022

Trailing comma is not added to multi-line function invocations In code such as.

class Foo {

  public static function create(): static {
    return new static(
      'foo',
      'bar'
    );
  }

}

I expect a comma to be added directly after 'bar', such that code becomes:

class Foo {

  public static function create(): static {
    return new static(
      'foo',
      'bar',
    );
  }

}

@dpi
Copy link
Contributor Author

dpi commented May 11, 2022

Using return new self() did work however.

kubawerlos
kubawerlos requested changes May 12, 2022
View reviewed changes
Copy link
Member

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

@dpi nice catch, can you add some test to prevent regression?

@SpacePossum SpacePossum changed the title Comma not added to multiline new static TrailingCommaInMultilineFixer - Add comma to multiline new static May 13, 2022
@SpacePossum SpacePossum added the RTM Ready To Merge label May 13, 2022
@coveralls
Copy link

coveralls commented May 13, 2022 •
edited
Loading

Coverage Status

Coverage remained the same at 92.938% when pulling 0615210 on dpi:trailing-comma-multiline-new-static into 3b471b4 on FriendsOfPHP:master.

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented May 15, 2022

(my approval is subject to tests being added)

@SpacePossum SpacePossum removed the RTM Ready To Merge label May 15, 2022
@GrahamCampbell GrahamCampbell mentioned this pull request May 15, 2022
Closed
@keradus keradus added the status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required label May 16, 2022
@dpi
Copy link
Contributor Author

dpi commented May 30, 2022

Tests are in

@GrahamCampbell

@SpacePossum
Copy link
Contributor

SpacePossum commented May 30, 2022

@dpi thanks so much for adding the tests! The PR looks better and better. Currently the CI fails because of the CS of the changes, can you run the fixer itself on its code and have a look at the changes?

@dpi
Copy link
Contributor Author

dpi commented May 31, 2022

lint changed, its not clear to me how to get the other tests to run?

@SpacePossum SpacePossum added kind/enhancement and removed status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required labels May 31, 2022
@SpacePossum
Copy link
Contributor

SpacePossum commented May 31, 2022

seems your PR got hit by symfony/symfony#45361 , which is not related to your changes, so this CI failure doesn't need to be fixed in your PR

@dpi
Copy link
Contributor Author

dpi commented May 31, 2022

Huzzah!

@julienfalque julienfalque force-pushed the trailing-comma-multiline-new-static branch from d60786b to 0615210 Compare August 13, 2022 20:21
@julienfalque julienfalque changed the title TrailingCommaInMultilineFixer - Add comma to multiline new static minor: TrailingCommaInMultilineFixer - Add comma to multiline new static Aug 13, 2022
@julienfalque julienfalque merged commit 32cf09e into PHP-CS-Fixer:master Aug 13, 2022
@julienfalque
Copy link
Member

julienfalque commented Aug 13, 2022

Thank you @dpi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julienfalque julienfalque julienfalque approved these changes

@kubawerlos kubawerlos kubawerlos approved these changes

+2 more reviewers

@GrahamCampbell GrahamCampbell GrahamCampbell approved these changes

@SpacePossum SpacePossum SpacePossum approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@dpi @coveralls @GrahamCampbell @SpacePossum @julienfalque @kubawerlos @keradus