Add a check that associative arrays are multi-line - includes fixer #773

Open
wants to merge 2 commits into
from

Projects

None yet

2 participants

@jrfnl
Contributor
jrfnl commented Jan 6, 2017

For associative arrays, values should start on a new line.
https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#indentation

Detecting this was easy, creating the fixer.... not so much...
I got it working, but would not be surprised if I rewrite the fixer in a few months or so. I've tried to anticipate the most common situations, but the fixer will fail on arrays with very inconsistent spacing.
Think:

array(
	'key' => array( 'key1' => 'value', 'key2' => array(
		'another_value'
	) ),
)

Anticipating a comment on line 43 of the .fixed file: the fixer will add a trailing comma to arrays which it is triggered on (if needed), but will not do so on arrays on which it is not triggered - that is covered by another sniff.

Fixes #638

  • Allows for both long as well as short array syntax.
  • Includes unit tests.
  • Includes fixing the one file in the WPCS code base which did not comply with the rule.
@jrfnl jrfnl added this to the 0.11.0 milestone Jan 6, 2017
WordPress-Core/ruleset.xml
<!-- Covers rule: Note the comma after the last array item: this is recommended. -->
<rule ref="WordPress.Arrays.ArrayDeclaration">
<exclude name="WordPress.Arrays.ArrayDeclaration.SingleLineNotAllowed" />
</rule>
+ <!-- Covers rule: For associative arrays, values should start on a new line.
+ Also covers various array whitespace issues) -->
@JDGrimes
JDGrimes Jan 9, 2017 Contributor

Looks like the closing parenthesis is missing a match here.

@jrfnl
jrfnl Jan 10, 2017 Contributor

Fixed

// Test for fixing of extra whitespace.
$test = array( 1, 2 );
+
+$bad = array( 'key' => 'value' ); // Bad, each value of an associative array should start on a new line.
@JDGrimes
JDGrimes Jan 9, 2017 Contributor

I have always thought that when there are multiple values in the array, it should be split to have one value per line. When there is just one key => value pair, I don't think that it has to be multiline (although that should still be permissible, in case of long values that need to be wrapped). I do see that the docs actually say:

For associative arrays, values should start on a new line.

Though I'm a bit incredulous that that means for arrays with a single element.

@jrfnl
jrfnl Jan 10, 2017 edited Contributor

The handbook doesn't say anywhere that multi-value arrays should be multi-line (though I totally agree that that would be good practice).
It only has the bit we both quoted and that bit doesn't specify that it would only apply to multi-value associative arrays...

jrfnl added some commits Jan 10, 2017
@jrfnl jrfnl Add check that associative arrays are multi-line - includes fixer.
The check itself is quite simple, fixing this automatically however turned out to be a whole lot harder.
e586ebf
@jrfnl jrfnl Make the WPCS code base comply with the "associative array must be mu…
…lti-line" rule.

Only affected the `WordPress.WP.I18n` sniff.
9592cf3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment