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

feature: TrailingCommaInMultilineFixer - Add array destructuring support #6518

Conversation

SpacePossum
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jul 25, 2022

Coverage Status

Coverage increased (+0.006%) to 92.921% when pulling eb340fe on SpacePossum:master_TrailingCommaInMultilineFixer_add_array_destructuring into aafad72 on FriendsOfPHP:master.

@SpacePossum SpacePossum force-pushed the master_TrailingCommaInMultilineFixer_add_array_destructuring branch from 1406168 to 6a21a18 Compare July 25, 2022 06:57
Using the `@Symfony <./../../ruleSets/Symfony.rst>`_ rule set will enable the ``trailing_comma_in_multiline`` rule with the default config.
Using the `@Symfony <./../../ruleSets/Symfony.rst>`_ rule set will enable the ``trailing_comma_in_multiline`` rule with the config below:

``['elements' => ['array_destructuring', 'arrays', 'match']]``
Copy link
Member

Choose a reason for hiding this comment

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

The default value (as was previously used) is ['arrays'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new rule, as such there is no "previously used"?

Copy link
Member

Choose a reason for hiding this comment

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

This is a new rule

Not sure I follow you. What makes you think the rule is new?

Anyway, the @Symfony ruleset isn't new so it shouldn't change. But actually Symfony might be OK with this? @nicolas-grekas What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

trailing_comma_in_multiline rule is already on master, so it is not a new rule indeed

yet, the configuration of it for Sf ruleset changed - not sure if it's welcome by Sf community, did we checked if repo follows this new rule already, or checked with maintainers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confused this PR with another, sorry about that.
The changes on SF 5.4 would look like this:
https://github.com/SpacePossum/symfony/pull/2/files

doc/rules/basic/trailing_comma_in_multiline.rst Outdated Show resolved Hide resolved
src/Fixer/Basic/TrailingCommaInMultilineFixer.php Outdated Show resolved Hide resolved
@SpacePossum SpacePossum force-pushed the master_TrailingCommaInMultilineFixer_add_array_destructuring branch 3 times, most recently from a78178c to 3cf2793 Compare August 21, 2022 15:19
@SpacePossum
Copy link
Contributor Author

SpacePossum commented Aug 21, 2022

rebased on latest, fixed conflict, moved back some stuff (#6518 (comment)), no other changes

@jrmajor jrmajor mentioned this pull request Aug 21, 2022
8 tasks
@SpacePossum SpacePossum force-pushed the master_TrailingCommaInMultilineFixer_add_array_destructuring branch from 3cf2793 to 5537a92 Compare August 31, 2022 09:23
@SpacePossum
Copy link
Contributor Author

rebased on latest, no other changes

@SpacePossum SpacePossum force-pushed the master_TrailingCommaInMultilineFixer_add_array_destructuring branch from 5537a92 to eb340fe Compare September 5, 2022 09:48
@SpacePossum
Copy link
Contributor Author

rebased on latest, no other changes

@SpacePossum SpacePossum closed this Oct 3, 2022
@SpacePossum SpacePossum deleted the master_TrailingCommaInMultilineFixer_add_array_destructuring branch October 3, 2022 18:38
@Wirone Wirone mentioned this pull request Aug 23, 2023
16 tasks
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.

None yet

5 participants