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

binary_operator_spaces align option does not handle correctly array of array #4395

Closed
VincentLanglet opened this issue Apr 28, 2019 · 8 comments · Fixed by #6590
Closed

binary_operator_spaces align option does not handle correctly array of array #4395

VincentLanglet opened this issue Apr 28, 2019 · 8 comments · Fixed by #6590

Comments

@VincentLanglet
Copy link
Contributor

The PHP version you are using ($ php -v):

=> 7.1

PHP CS Fixer version you are using ($ php-cs-fixer -V):

=> 2.14.2

The command you use to run PHP CS Fixer:

=> php-cs-fixer fix ...

The configuration file you are using, if any:

'binary_operator_spaces' => [
        'default' => 'single_space',
        'operators' => [
            '=>' => 'align_single_space_minimal',
            '=' => null,
        ]
    ],

If applicable, please provide minimum samples of PHP code (as plain text, not screenshots):

  • before running PHP CS Fixer (no changes):
$structure = [
    'pdfVoucherPath'  => [
        'pdfVoucherExisting' => 'pdfVoucherExisting',
    ],
    'pdfAppendixPath' => [
        'pdfAppendixExisting' => 'pdfAppendixExisting',
    ],
    'pdfDiciPath'     => [],
    'pdfBonusPath'    => [
        'pdfBonusExisting' => 'pdfBonusExisting',
    ],
];
  • with unexpected changes applied when running PHP CS Fixer:
$structure = [
    'pdfVoucherPath'  => [
        'pdfVoucherExisting' => 'pdfVoucherExisting',
    ],
    'pdfAppendixPath' => [
        'pdfAppendixExisting' => 'pdfAppendixExisting',
    ],
    'pdfDiciPath'  => [],
    'pdfBonusPath' => [
        'pdfBonusExisting' => 'pdfBonusExisting',
    ],
];
  • with the changes you expected instead:
$structure = [
    'pdfVoucherPath'  => [
        'pdfVoucherExisting' => 'pdfVoucherExisting',
    ],
    'pdfAppendixPath' => [
        'pdfAppendixExisting' => 'pdfAppendixExisting',
    ],
    'pdfDiciPath'     => [],
    'pdfBonusPath'    => [
        'pdfBonusExisting' => 'pdfBonusExisting',
    ],
];

Basically, I expect the definition of align_single_space and align_single_space_minimal to align ALL => of an array.

@VincentLanglet
Copy link
Contributor Author

Another wrong behaviour

This following code

        $enrichedRecommendableContractType = array_merge(
            $recommendableContractType,
            ['kidAccepted' => false],
            ['termsAndConditionsAccepted' => false]
        );

Is fixed to

        $enrichedRecommendableContractType = array_merge(
            $recommendableContractType,
            ['kidAccepted'                => false],
            ['termsAndConditionsAccepted' => false]
        );

But this following code is inchanged

        $enrichedRecommendableContractType = array_merge(
            ['kidAccepted' => false],
            ['termsAndConditionsAccepted' => false]
        );

I expect both code to be untouched

@VincentLanglet
Copy link
Contributor Author

After more investigation, the rule is made to align the => in consecutive lines, even if they are on different array.
On the contrary, two => in the same array are not aligned if there is a blank line or a line of comment between them.

I don't think this should be the behavior for =>. @keradus @kubawerlos
Is there a way to introduce a different behavior ? Another option ?

@Vinorcola
Copy link

Hi, I have the same issue.

Maybe, one way to fix it is to consider each array as a scope, like it is done here: #6559 ?

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Aug 19, 2022

Hi, I have the same issue.

Maybe, one way to fix it is to consider each array as a scope, like it is done here: #6559 ?

I think it's already consider as the same scope.

$structure = [
    'pdfVoucherPath'  => [
        'pdfVoucherExisting' => 'pdfVoucherExisting',
    ],
    'pdfAppendixPath' => [
        'pdfAppendixExisting' => 'pdfAppendixExisting',
    ],
    'pdfDiciPath'     => [],
    'pdfBonusPath'    => [
        'pdfBonusExisting' => 'pdfBonusExisting',
    ],
];

is changed to

$structure = [
    'pdfVoucherPath'  %1% [
        'pdfVoucherExisting' %2% 'pdfVoucherExisting',
    ],
    'pdfAppendixPath' %1% [
        'pdfAppendixExisting' %3% 'pdfAppendixExisting',
    ],
    'pdfDiciPath'     %1% [],
    'pdfBonusPath'    %1% [
        'pdfBonusExisting' %4% 'pdfBonusExisting',
    ],
];

But the method which aligns operator are only working on the consecutive lines with the same scope.
See https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/src/Fixer/Operator/BinaryOperatorSpacesFixer.php#L746-L758

Because

$a = 1;
$bb = 2;

$c = 3;
$ddd = 4;

is expected to be changed to

$a  = 1;
$bb = 2;

$c   = 3;
$ddd = 4;

So the question would be, should be consider => as specific.
But this can be considered as an important change, and would maybe require a lot of approval.

@Vinorcola
Copy link

Should there be a specific rule for array alignment then?

@VincentLanglet
Copy link
Contributor Author

This is easy to implements VincentLanglet@4bbffd8

But it requires the fixer to works perfectly (or it will considered => out of the array as in the same scope), so it require at least #6559 to be merged first.

I don't know how much people are expecting the same behavior, ie

$a = [
    1 => [
    ],
    1111111111111 => 2
];

be fixed to

$a = [
    1             => [
    ],
    1111111111111 => 2
];

If we introduce a new configuration, it can be weird having an option which is only working for =>.
Maybe the first step would be to split the BinaryOperatorFixer.
If we look at the implementation there is

  • Implementation for every operator except =>.
  • Implementation for =>.

@julienfalque or @SpacePossum might be good advice on this.

@Vinorcola
Copy link

Vinorcola commented Aug 19, 2022

The question will also arise for enum alignments (It is a bit out off topic here).

For example, I'd like my enums to be aligned this way:

<?php

enum OrderStatus: string
{
    case ESTIMATION = 'estimation';
    case DRAFT      = 'draft';
    case ORDERED    = 'ordered';
    case DELIVERED  = 'delivered';
    case PAYED      = 'payed';
    case CANCELED   = 'canceled';
}

But this is not possible with the binary_operator_spaces rule, without endind up with aligned assignments (which I don't wan't):

<?php

$someVar = 'xyz';
$a       = 123;

So at some point, we will need a specific rule for enum alignments. A specific rule for array alignments make sense...

EDIT: Question will also arise for match blocks

@VincentLanglet
Copy link
Contributor Author

The question will also arise for enum alignments (It is a bit out off topic here).

For example, I'd like my enums to be aligned this way:

<?php

enum OrderStatus: string
{
    case ESTIMATION = 'estimation';
    case DRAFT      = 'draft';
    case ORDERED    = 'ordered';
    case DELIVERED  = 'delivered';
    case PAYED      = 'payed';
    case CANCELED   = 'canceled';
}

But this is not possible with the binary_operator_spaces rule, without endind up with aligned assignments (which I don't wan't):

<?php

$someVar = 'xyz';
$a       = 123;

So at some point, we will need a specific rule for enum alignments.

This is too specific/complicated. There is no context used in the operatorFixer right now.
You might argue that we could have a context for enum but people could ask the same for class constant

public const Foo      = 1;
public const SuperFoo = 2;

The only thing that can be done IMHO, is #4396

A specific rule for array alignments make sense...

EDIT: Question will also arise for match blocks

Array and match will be considered as the same for the same reason. We're just looking for =>.

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 a pull request may close this issue.

2 participants