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

Proposal: Add rules about formatting of the spread operator #1762

Closed
jrfnl opened this issue Jul 10, 2019 · 7 comments · Fixed by #2100
Closed

Proposal: Add rules about formatting of the spread operator #1762

jrfnl opened this issue Jul 10, 2019 · 7 comments · Fixed by #2100

Comments

@jrfnl
Copy link
Member

jrfnl commented Jul 10, 2019

Is your feature request related to a problem?

PHP 5.6 introduced the spread operator ... for packing arguments in function declarations (variadic functions) and unpacking them in function calls.

PHP 7.4 will introduce unpacking of arrays using the spread operator.

Refs:

At this moment WPCS has no formatting rules for the spread operator.

Describe the solution you'd like

I would like to propose adding the following rules regarding the spread operator to the WP Coding Standards handbook:

  • Rule 1: There should be one space, or a new line + indentation, before the spread operator.
  • Rule 2: There should be no space (or comments for that matter) between the spread operator and the variable/function call it applies to.
  • Rule 3: When combining the spread operator with the reference operator, there should be no space between them.
// Correct.
function foo( &...$spread ) {
	bar( ...$spread );

	bar(
		[ ...$foo ],
		...array_values( $keyed_array )
	);
}

// Incorrect.
function fool( &   ... $spread ) {
	bar(...
             $spread );

	bar(
		[...  $foo ],.../*comment*/array_values( $keyed_array )
	);
}

If this proposal is accepted, we can then add sniff(s) to WPCS to check these rules automatically and - except for issues with comments - auto-fix them.

Notes for the WPCS Implementation:

  • Rule 1 is generally already checked via other sniffs, such as sniffs checking the spacing of function declaration parameters, function call parameters, array items.
  • Rule 2 warrants a new sniff.
    For the function declaration part, PSR-12 demands the same - no space between the spread operator and the variable/function call -. For usage within array declarations and function calls, PSR-12 has no explicit opinion.
    With that in mind, I have pulled a comprehensive sniff to PHPCS itself which - once merged - could be used to cover this rule.
    See: New Generic.WhiteSpace.SpreadOperatorSpacingAfter sniff (PSR12 related) squizlabs/PHP_CodeSniffer#2548
  • Rule 3: AFAICS there is no sniff available currently to cover this, but PSR12 demands the same, so an upstream sniff to cover this can be expected in the near future.

/cc @pento

@dingo-d
Copy link
Member

dingo-d commented Jul 10, 2019

Should we put in this sniff the check, in the case of multiple arguments used with the spread operator, that the variadic argument is always placed on the last place?

// Correct.
function foo( $required, $optional = null, ...$variadic ) {
    // Code goes here
}

// Incorrect. Will output a fatal error.
function foo( $required, ...$variadic, $optional = null ) {
    // Code goes here
}

@jrfnl
Copy link
Member Author

jrfnl commented Jul 10, 2019

Should we put in this sniff the check, in the case of multiple arguments used with the spread operator, that the variadic argument is always placed on the last place?

Good point. In my opinion, no, this should not be covered here. This topic is about the formatting of the spread operator, i.e. whitespace and such.

Parse errors are covered by the Generic.PHP.Syntax sniff which is included in WordPress-Extra.

If we would want to check the order of parameters, I think a more generic CodeAnalysis.FunctionDeclarationParameterOrder sniff would be more appropriate.

Such a sniff could then check:

  • That optional arguments are always declared after required parameters.
  • That variadic parameters are always declared as the last parameter.
  • That a function declaration only has one variadic parameter.

If such a sniff would be desired, a separate issue should be opened about it and we'd need to check if anything of the above is already covered and if there are any upstream sniffs available we could use for this.

@pento
Copy link
Member

pento commented Jul 12, 2019

I agree with rules 1, 2, and 3. 🎉

@jrfnl
Copy link
Member Author

jrfnl commented Sep 20, 2019

FYI: The upstream PR for rule 2 has been merged and a sniff for rule 3 has also been added. Both will be included in PHPCS 3.5.0.

@jrfnl jrfnl added the Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). label Sep 20, 2019
@jrfnl
Copy link
Member Author

jrfnl commented Sep 27, 2019

FYI: PHPCS 3.5.0 has been released.

@jrfnl
Copy link
Member Author

jrfnl commented Dec 1, 2019

FYI: Based on the PHPCS 3.5.0 changelog, rule 3 is covered via the Squiz.Functions.FunctionDeclarationArgumentSpacing which is already included in the WordPress-Core ruleset.

Squiz.Functions.FunctionDeclarationArgumentSpacing now checks for no space after a reference operator

Similar for rule 2, though only in the case of function declarations, so adding the PHPCS 3.5.0 Generic.WhiteSpace.SpreadOperatorSpacingAfter sniff is still needed.

Squiz.Functions.FunctionDeclarationArgumentSpacing now checks for no space after a variadic operator

Src: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.5.0

@jrfnl
Copy link
Member Author

jrfnl commented Dec 1, 2019

If we would want to check the order of parameters, I think a more generic CodeAnalysis.FunctionDeclarationParameterOrder sniff would be more appropriate.

@dingo-d The PEAR.Functions.ValidDefaultValue sniff partially covers this. Also see: squizlabs/PHP_CodeSniffer#2719

Note: that sniff is currently not included in any of the WPCS rulesets.

@jrfnl jrfnl added this to the 3.0.0 milestone Mar 14, 2020
@jrfnl jrfnl removed the Status: Requirement not met On hold until WPCS supports a particular version of a dependency (PHP, PHP_CodeSniffer, etc.). label Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants