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: Add strict option to array indentation #7846

Closed

Conversation

VincentLanglet
Copy link
Contributor

Hi @Wirone,

When working on #7813, I discovered that:

  • I need the array_indentation to be use if someone wants to use single_expression_per_line for arrays
  • The array_indentation is not added to any PSR sets.

If we wants to add the single_expression_per_line for arrays to the PERCS 2, which would resolve the part

When the array declaration is split across multiple lines, the opening bracket MUST be placed on the same line as the equals sign. The closing bracket MUST be placed on the next line after the last value. There MUST NOT be more than one value assignment per line. Value assignments MAY use a single line or multiple lines.

we will need to indent correctly the array. So we will need array_indentation in PERCS 2 too or at least in a less strict version: "Each element of an array must be indented AT LEAST once."

When looking at PSR12 and PSR2, there is the following section

Code MUST use an indent of 4 spaces for each indent level, and MUST NOT use tabs for indenting.

I think the array_indentation is going in this direction and therefore could be added to the PSR2 ruleset (with the option strict => false to minimize the impact). This would be similar to the statement_indentation which is already added to the PSR2 ruleset.

WDYT ?

  • Should we add array_indentation with strict => false to the PSR2 ?
  • Should we add array_indentation with strict => false to the PERCS 2 only ?
  • Should we add array_indentation with strict => true to the PERCS 2 ?

For the records,

  • PHPCsFixer already use the array_indentation with strict => true (since the option didn't exist)
  • Symfony don't use the array_indentation but with strict => false there are some fixes in the code base which seems to be typo most of the time (like 3 spaces instead of 4) ; there was way more fixes with strict => true, so maybe it's voluntary to sometime use more indentations.

@VincentLanglet VincentLanglet changed the title Add strict option to array indentation feat: Add strict option to array indentation Feb 18, 2024
@coveralls
Copy link

coveralls commented Feb 18, 2024

Coverage Status

coverage: 96.03% (+1.2%) from 94.805%
when pulling a59f1b5 on VincentLanglet:array-indentation
into 0edbfb9 on PHP-CS-Fixer:master.

@VincentLanglet
Copy link
Contributor Author

WDYT ?

  • Should we add array_indentation with strict => false to the PSR2 ?
  • Should we add array_indentation with strict => false to the PERCS 2 only ?
  • Should we add array_indentation with strict => true to the PERCS 2 ?

Friendly ping @Wirone @kubawerlos :)

This would help me to make #7813 useful

@@ -35,6 +39,7 @@ public function getDefinition(): FixerDefinitionInterface
'Each element of an array must be indented exactly once.',
[
new CodeSample("<?php\n\$foo = [\n 'bar' => [\n 'baz' => true,\n ],\n];\n"),
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I don't get what this option does, are you able to update the samples to have the same input code with both strict options to see the difference?

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 updated the code sample, it more clear now ?

The difference is the same than
SINGLE_SPACE vs AT_LEAST_SINGLE_SPACE for the BinaryOperatorSpacesFixer.

Currently, array_indentation enforce EXACTLY one indentation.
When looking at PSR12, it enforce indentation but not "exactly" one, so it could be considered to add the rule with the strict => false option to enforce AT LEAST one indentation.

This option would be also useful for SymfonyRuleset since they use indentation, but sometimes use more than one indentation.

In my case this would be useful for #7813 because I need a fixer to resolve indentation after mine but the array_indentation is too strict atm to be added in the PSR ruleset.

@julienfalque
Copy link
Member

I don't find the "at least" words in the "Arrays" section of PER CS 2.0:

Array declarations MAY be split across multiple lines, where each subsequent line is indented once.

PSR-12 does not specifically provide guidelines for arrays and PSR-2 does not use the words "at least" at all. Maybe I'm missing something but I don't see a need for this option. I would add the rule to the @PER-CS2.0 ruleset without introducing any option.

Also, formatting arrays like e.g.

[
    'foo',
        'bar',
]

seems a bit off. Do you know actual use cases?

@VincentLanglet
Copy link
Contributor Author

I don't find the "at least" words in the "Arrays" section of PER CS 2.0:

Array declarations MAY be split across multiple lines, where each subsequent line is indented once.

Yes, for PERCS 2.0 it should be strict
So I created #7881 for this.

PSR-12 does not specifically provide guidelines for arrays and PSR-2 does not use the words "at least" at all. Maybe I'm missing something but I don't see a need for this option. I would add the rule to the @PER-CS2.0 ruleset without introducing any option.

Also, formatting arrays like e.g.

[
    'foo',
        'bar',
]

seems a bit off. Do you know actual use cases?

Not really, I thought this would help to add the array_indentation rule to PSR/PERCS, but that's maybe not a good idea.
Let's just go with #7881

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