Add option to NormalizedArrays.Arrays.ArrayBraceSpacing to allow single- or multi-line empty arrays

[Daimona]

Is your feature request related to a problem?

The NormalizedArrays.Arrays.ArrayBraceSpacing sniff takes a spacesWhenEmpty property that determines the spacing to enforce for empty arrays. This can be set to an arbitrary number of space characters, or 'newline', but not both. I would like to be able to allow both of these styles:

$a1 = [];
$a2 = [
];

Where the second is particularly useful when the value is expected to change, because additions and removals to that array will show more cleanly in git diffs and history.

However, I don't think there's a way to configure the ArrayBraceSpacing sniff so that both styles are allowed.

Describe the solution you'd like

I would like to be able to specify different spacing rules for empty arrays depending on whether they're single-line or multi-line. I think there might be a few ways to achieve that (separate config values for the two cases; a new config flag to decide whether "empty" only applies to the single-line case; etc.). I do not have any preferences as to how this is implemented, as long as it's possible to specify the intended behaviour described above.

• [ ] I intend to create a pull request to implement this feature.

[jrfnl]

@Daimona I appreciate your suggestion, but have no intention of adding support for this.

Sniffs, and even coding standards in general, are about consistency. You can choose what you want to make consistent (i.e. which sniffs/error codes to include), but enforcing inconsistency is not something I will support.

I suggest you exclude the EmptyArraySpacing errorcode, then both styles are automatically allowed as the spacing for empty arrays will no longer be checked.

[Daimona]

Sniffs, and even coding standards in general, are about consistency. You can choose what you want to make consistent (i.e. which sniffs/error codes to include), but enforcing inconsistency is not something I will support.

Thank you, I understand that. I believe that allowing a certain degree of freedom makes for a nicer developer experience, especially when the code base is large and/or the group of developers is very diverse. And I would argue that sometimes, inconsistency is a good thing :-) Even within the limited context of this sniff, single-line arrays are (or can be) treated inconsistently depending on whether they're empty, but I think we all agree that this is good.

That said, again, I understand your reasoning and that's perfectly fine! I have a question, though: would you be willing to accept a PR that breaks ArrayBraceSpacingSniff::process down into multiple methods, so that I could more easily extend the sniff in our standard to implement the behaviour described above?

I suggest you exclude the EmptyArraySpacing errorcode, then both styles are automatically allowed as the spacing for empty arrays will no longer be checked.

Yup, I did that (actually setting spacesWhenEmpty to false), but I really wanted to enable the empty array checks as well, to catch things like multiple spaces.

[jrfnl]

Sniffs, and even coding standards in general, are about consistency. You can choose what you want to make consistent (i.e. which sniffs/error codes to include), but enforcing inconsistency is not something I will support.

Thank you, I understand that. I believe that allowing a certain degree of freedom makes for a nicer developer experience, especially when the code base is large and/or the group of developers is very diverse.

Oh, I totally agree, which is why I try to have modular sniffs with different error codes for each case, to allow people to include/exclude individual checks as easily as possible.

I have a question, though: would you be willing to accept a PR that breaks ArrayBraceSpacingSniff::process down into multiple methods, so that I could more easily extend the sniff in our standard to implement the behaviour described above?

Unfortunately that would not work as all sniffs in this package are final.

The PHPCS native autoloader (which is used to load the sniffs) does not play nice with sniffs extending each other and I have seen too many problems with that, so removing the final keyword is not up for discussion.

A more appropriate solution would be to:

1. Exclude the error code(s) from this sniff which you don't want.
2. Write a separate sniff for your own standard which only examines empty arrays and enforces the "no multiple spaces" rule you want.

As an alternatively for [2], you can also have a separate sniff which does a prelim check for empty arrays + enforces the "no multiple spaces" rule and bows out after that for all empty arrays and passes off to an instance of the ArrayBraceSpacingSniff created within your own sniff for everything else.

Both solutions would work and would not require changes in this repo.

Does that help ?

[Daimona]

Unfortunately that would not work as all sniffs in this package are final.

Oooops, I had not realized that.

1. Exclude the error code(s) from this sniff which you don't want.
2. Write a separate sniff for your own standard which only examines empty arrays and enforces the "no multiple spaces" rule you want.

As an alternatively for [2], you can also have a separate sniff which does a prelim check for empty arrays + enforces the "no multiple spaces" rule and bows out after that for all empty arrays and passes off to an instance of the ArrayBraceSpacingSniff created within your own sniff for everything else.

Both solutions would work and would not require changes in this repo.

Does that help ?

Yup, I will look into this, thank you!