PHP_CodeSniffer icon indicating copy to clipboard operation
PHP_CodeSniffer copied to clipboard

PEAR/FunctionDeclaration: examine arrow function declarations + fix fixer conflict

Open jrfnl opened this issue 3 years ago • 7 comments

PEAR/FunctionDeclaration: examine arrow function declarations

PHP 7.4 arrow functions were not yet being taken into account for this sniff.

Note: I've explicitly left the sniff to not have an opinion on the formatting for "arrow on same line/new line".

Fixed now. Includes unit tests.

Squiz/MultiLineFunctionDeclaration: add tests for arrow function declarations

The Squiz sniff inherits the change from the PEAR.Functions.FunctionDeclaration sniff.

This commit adds tests to document the behaviour and safeguard the inherited change.

~~PEAR/FunctionDeclaration: prevent fixer conflict~~

~~If a return type declaration was not confined to one line, the sniff could have a fixer conflict with itself. The fixer would also potentially remove a close curly on the same line, causing parse errors.~~

~~Fixed now. The diff will be most straight forward to review while ignoring whitespace changes. Includes unit tests.~~

jrfnl avatar Sep 09 '22 14:09 jrfnl

I'm not sure about enforcing the multi-line rules for arrow functions. I think the spacing after FUNCTION and FN is harmless enough given arrow functions are mostly single-line, but anything that looks into how multi-line arrow functions are formatted feels like making up a standard that doesn't exist.

gsherwood avatar Sep 10 '22 06:09 gsherwood

I'm not sure about enforcing the multi-line rules for arrow functions. I think the spacing after FUNCTION and FN is harmless enough given arrow functions are mostly single-line, but anything that looks into how multi-line arrow functions are formatted feels like making up a standard that doesn't exist.

The changes I've made only kick in when arrow functions already use some sort of multi-line format for the parameters (not the function body).

I agree that arrow functions will mostly be single-line, so chances that this sniff will kick in are slim anyway, but it felt like an oversight leading to inconsistency to not examine and fix the parameter lay-out when it is multi-line.

jrfnl avatar Sep 10 '22 07:09 jrfnl

I've added two more commits to this PR and updated the description to include those.

jrfnl avatar Sep 11 '22 02:09 jrfnl

I've moved the last commit (preventing a fixer conflict for function declarations with return types) to PR #3739 as that's a straight forward bug fix and the discussion on whether on not the sniff should examine arrow functions is blocking that bug fix from going in.

If no decision is taken about the sniff and arrow functions by the time #3739 has been merged, I will rebase this PR after the merge.

jrfnl avatar Jan 05 '23 03:01 jrfnl

Needs checking if these sniffs are used by PSR12 and if so, if PSR-PER has any additional information.

jrfnl avatar May 04 '23 07:05 jrfnl

Rebased without changed to get rid of conflicts after the merge of #3787.

jrfnl avatar May 04 '23 08:05 jrfnl

Needs checking if these sniffs are used by PSR12 and if so, if PSR-PER has any additional information.

I have checked and this PR will need to be changed/updated to be in line with PSR-PER. I'm moving this PR to draft until it has been updated.

jrfnl avatar May 18 '23 11:05 jrfnl