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

feat: update Symfony.trailing_comma_in_multiline config #7825

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seb-jean
Copy link

@seb-jean seb-jean commented Feb 6, 2024

No description provided.

@coveralls
Copy link

Coverage Status

coverage: 94.772%. remained the same
when pulling 0cf99b7 on seb-jean:patch-1
into 36c7a65 on PHP-CS-Fixer:master.

@Wirone
Copy link
Member

Wirone commented Feb 7, 2024

Hi @seb-jean, is this change approved by Symfony maintainers? Or maybe there's some conversation about it?

Anyway, as you can see it fails because we still support PHP 7.4 and trailing comma in function parameters' list is supported from 8.0 up.

@seb-jean
Copy link
Author

seb-jean commented Feb 7, 2024

This is to have the same value as https://github.com/symfony/symfony/blob/7.1/.php-cs-fixer.dist.php#L43

@Wirone
Copy link
Member

Wirone commented Feb 7, 2024

But Symfony 7.1 requires a higher PHP version and can have this in local config, while it can't be added as-is to the ruleset in this repository because it's tested on 7.4-8.4. You need to make conditional rules, adding parameters only for 8.0+.

@nicolas-grekas are you OK with this change?

@nicolas-grekas
Copy link

Yes, this changes makes sense (but I didn't think about the PHP-version issue - maybe "parameters" should be turned on only on PHP 8 that's what you mean?)

@Wirone
Copy link
Member

Wirone commented Feb 8, 2024

Yes, exactly, it should be dynamic, based on PHP version. Thanks @nicolas-grekas for confirming 🙂.

@seb-jean are you going to make CI green?

@seb-jean
Copy link
Author

seb-jean commented Feb 8, 2024

I do not know how to do. Sorry.

@Wirone
Copy link
Member

Wirone commented Feb 8, 2024

@seb-jean you should:

  • modify ruleset in a way where trailing_comma_in_multiline.elements.parameters is added dynamically, if runtime's version is 8.0+ (this will fix "parameters" option can only be enabled with PHP 8.0+. in 7.4 jobs). So in SymfonySet::getRules()instead of returning array immediately you need to assign it to variable, use 'trailing_comma_in_multiline' => ['elements' => ['arrays', 'match']], and then $rules['trailing_comma_in_multiline']['elements'][] = 'parameters'; if PHP 8.0+, then return rules.
  • run composer cs:fix which will apply the rule to the Fixer's codebase (as we use @Symfony ruleset as a base for @PhpCsFixer ruleset, so this change affects our code). In general you should be able to run composer qa without any error, then it's ready 😉.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants