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

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Sep 9, 2022

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.

@gsherwood
Copy link
Member

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 gsherwood added this to Idea Bank in PHPCS v3 Development via automation Sep 10, 2022
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 10, 2022

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 jrfnl force-pushed the feature/pear-functiondeclarations-handle-arrow-functions branch 2 times, most recently from 87aa11f to e922ddf Compare September 11, 2022 02:25
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 11, 2022

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

@jrfnl jrfnl changed the title PEAR/FunctionDeclaration: examine arrow function declarations PEAR/FunctionDeclaration: examine arrow function declarations + fix fixer conflict Sep 11, 2022
@jrfnl jrfnl force-pushed the feature/pear-functiondeclarations-handle-arrow-functions branch from e922ddf to fa2f2df Compare September 11, 2022 02:32
@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 5, 2023

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
Copy link
Contributor Author

jrfnl commented May 4, 2023

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

jrfnl added 3 commits May 4, 2023 10:44
PHP 7.4 arrow functions were not yet being taken into account for this sniff.

Fixed now.
Includes unit tests.
…arations

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.
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 jrfnl force-pushed the feature/pear-functiondeclarations-handle-arrow-functions branch from fa2f2df to d479811 Compare May 4, 2023 08:51
@jrfnl jrfnl closed this May 4, 2023
PHPCS v3 Development automation moved this from Idea Bank to Ready for Release May 4, 2023
@jrfnl jrfnl reopened this May 4, 2023
@jrfnl
Copy link
Contributor Author

jrfnl commented May 4, 2023

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

@jrfnl jrfnl moved this from Ready for Release to Idea Bank in PHPCS v3 Development May 4, 2023
@jrfnl
Copy link
Contributor Author

jrfnl commented May 18, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants