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

Array declaration spacing sniff #1456

Open
michnovka opened this issue Oct 19, 2022 · 13 comments
Open

Array declaration spacing sniff #1456

michnovka opened this issue Oct 19, 2022 · 13 comments

Comments

@michnovka
Copy link
Contributor

michnovka commented Oct 19, 2022

#[DiscriminatorMap(['cc' => Component\ConcreteComponent::class,
        'cd' => Decorator\ConcreteDecorator::class])]
abstract class Component

This looks wrong to me and currently there are no sniffs targeting this.

What do you think about SlevomatCodingStandard.Attributes.RequireAttributeParamsInline?

#[DiscriminatorMap(['cc' => Component\ConcreteComponent::class, 'cd' => Decorator\ConcreteDecorator::class])]
 abstract class Component

Or we can have a sniff that enforces

#[DiscriminatorMap([
    'cc' => Component\ConcreteComponent::class,
    'cd' => Decorator\ConcreteDecorator::class,
])]
abstract class Component

btw, is SlevomatCodingStandard.Arrays.TrailingArrayComma applicable here? EDIT: it does apply and does work as expected

@kukulich
Copy link
Contributor

I think it should be solved by standard sniffs because it's constructor call.

@michnovka
Copy link
Contributor Author

@kukulich ok, so it looks like what i am missing is actually some Array sniff, like we have SlevomatCodingStandard.Arrays.MultiLineArrayEndBracketPlacement I would be looking for SlevomatCodingStandard.Arrays.MultiLineArrayStartBracketPlacement or SlevomatCodingStandard.Arrays.MultiLineArrayFirstItemPlacement

@kukulich
Copy link
Contributor

There are sniff in PHPCS itself, like Generic.Arrays.ArrayIndent, that should solve this - I think.

@michnovka
Copy link
Contributor Author

@kukulich I dont know, I have this sniff enabled and it doesnt complain about stuff like

#[ORM\Index(columns: ['phone_prefix',
    'sed',
])]

@kukulich
Copy link
Contributor

@michnovka The sniff probably does not have the implementation :) However I think it should be it's job.

@michnovka
Copy link
Contributor Author

@kukulich I have found this rule does what I need:

    <rule ref="WordPress.Arrays.ArrayDeclarationSpacing">
        <exclude name="WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceAfterArrayOpener"/>
        <exclude name="WordPress.Arrays.ArrayDeclarationSpacing.NoSpaceBeforeArrayCloser"/>
    </rule>

@michnovka
Copy link
Contributor Author

@kukulich the WP sniffs are very opinionated and change (as the name suggests WordPress.Arrays.ArrayDeclarationSpacing means -> use whatever WP standard is now accepted for array declaration spacing. There was case when this very standard changed

This sniff also does so many things:

Enforces WordPress array spacing format.

  • Check for no space between array keyword and array opener.
  • Check for no space between the parentheses of an empty array.
  • Checks for one space after the array opener / before the array closer in single-line arrays.
  • Checks that associative arrays are multi-line.
  • Checks that each array item in a multi-line array starts on a new line.
  • Checks that the array closer in a multi-line array is on a new line.

Slevomat CS on the other hands has an approach to name sniffs properly and have one sniff to one thing only.

Would you be interested in bringing this sniff as multiple sniffs under Slevomat CS?

@michnovka michnovka changed the title Arrays inside attributes Array declaration spacing sniff Oct 19, 2022
@kukulich
Copy link
Contributor

Yes, it's ok to add such sniff - but check if the sniff does not already exist in PHPCS itself. There are some sniffs like Squiz.Arrays.ArrayDeclaration or Generic.Arrays.ArrayIndent.

@michnovka
Copy link
Contributor Author

michnovka commented Oct 19, 2022

@kukulich seems that Squiz.Arrays.ArrayDeclaration has lot of the things provided by WP, but I am still missing

Checks that each array item in a multi-line array starts on a new line.

So lets add this one, I would call it SlevomatCodingStandard.Arrays.RequireMultilineStartsOnNewLine. Thoughts?

@michnovka
Copy link
Contributor Author

Also missing

Checks that associative arrays are multi-line.

So that might be another sniff Id create.

I will not have time for this until mid November, so lets use that time to finish discussion and specs here. maybe somebody will find the sniffs burried somewhere in the Squiz CS.

@kukulich
Copy link
Contributor

We should prepare list of missing features so we can create a propose about the sniffs.

@michnovka
Copy link
Contributor Author

michnovka commented Nov 10, 2022

Checks that associative arrays are multi-line. => DOES NOT EXIST
Checks that multiple-value associative arrays are multi-line. => Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed
Checks that each array item in a multi-line array starts on a new line. => Squiz.Arrays.ArrayDeclaration.IndexNoNewline

Check for no space between the parentheses of an empty array => SlevomatCodingStandard.Arrays.SingleLineArrayWhitespace.SpaceInEmptyArray
Checks for one space after the array opener / before the array closer in single-line arrays. => SlevomatCodingStandard.Arrays.SingleLineArrayWhitespace.SpaceAfterArrayOpen
Checks that the array closer in a multi-line array is on a new line. => Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine


therefore what we miss is

Checks that associative arrays are multi-line.

so that

$array = ['some_key',];
$arrayAssoc = ['some_key' => 'some_value',];

would be invalid, and it would be fixed to

$array = ['some_key',];
$array = [
    'some_key' => 'some_value',
];

I propose: SlevomatCodingStandard.Arrays.RequireAssociativeArraysMultiLine

@kukulich
Copy link
Contributor

I propose: SlevomatCodingStandard.Arrays.RequireAssociativeArraysMultiLine

Looks good to me. Just to be consistent with other sniff, it should probably be RequireMultiLineAssociativeArray (yes, naming is hard)

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

No branches or pull requests

2 participants