Skip to content

Add rule to ensure Arrow Functions declaration format #175

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

Merged
merged 1 commit into from
May 17, 2020

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Apr 18, 2020

This new rule enforces format for already declared arrow functions (this sniff
does not convert anonymous function to arrow functions).


Note for reviewers: the idea of having 1 space set for spacesCountAfterKeyword is that we follow the current standards for function calls.

Pool on Twitter for reference: https://twitter.com/carusogabriel/status/1251576089497010177

@carusogabriel carusogabriel added this to the 8.0.0 milestone Apr 18, 2020
@carusogabriel carusogabriel force-pushed the new-rule/arrow-functions-declaration branch 2 times, most recently from 03de28b to 0324f09 Compare April 18, 2020 19:46
WyriHaximus
WyriHaximus previously approved these changes Apr 19, 2020
This new rule enforces format for already declared `arrow functions` (this sniff
does not convert anonymous function to arrow functions).
@carusogabriel carusogabriel force-pushed the new-rule/arrow-functions-declaration branch from 0324f09 to 7e99a44 Compare May 2, 2020 16:17
@carusogabriel carusogabriel marked this pull request as ready for review May 2, 2020 16:25
@carusogabriel carusogabriel requested a review from a team as a code owner May 2, 2020 16:25
@@ -4,6 +4,7 @@ PHP CODE SNIFFER REPORT SUMMARY
FILE ERRORS WARNINGS
----------------------------------------------------------------------
tests/input/array_indentation.php 10 0
tests/input/arrow-functions-format.php 10 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors:

FILE: tests/input/arrow-functions-format.php
-------------------------------------------------------------------------------------------------
FOUND 10 ERRORS AFFECTING 7 LINES
-------------------------------------------------------------------------------------------------
  5 | ERROR | [x] Closure not using "$this" should be declared static.
  7 | ERROR | [x] Useless parentheses.
  9 | ERROR | [x] There must be no whitespace between closing parenthesis and return type colon.
 11 | ERROR | [x] Expected 0 spaces between argument "$a" and comma; 1 found
 13 | ERROR | [x] There must be exactly 1 whitespace after "fn" keyword.
 13 | ERROR | [x] There must be exactly 1 whitespace before =>.
 13 | ERROR | [x] There must be exactly 1 whitespace after =>.
 15 | ERROR | [x] There must be exactly 1 whitespace before =>.
 15 | ERROR | [x] There must be exactly 1 whitespace after =>.
 40 | ERROR | [x] Multi-line arrays must have a trailing comma after the last element.
-------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 10 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------

Time: 139ms; Memory: 10MB

malarzm
malarzm previously requested changes May 2, 2020
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer fn($x) which is also the winning option from your twitter poll

@SenseException
Copy link
Member

Should we have a different space handling compared to anonymous functions?

@ostrolucky
Copy link
Member

There is also an option to change spacing of normal closures, so we can have no space in both. I would welcome that personally.

@carusogabriel
Copy link
Contributor Author

carusogabriel commented May 4, 2020

@ostrolucky We can't, the closures' spacing rules follow PSR-12, and that's another argument to enforce a space with fn.

@carusogabriel carusogabriel removed the WIP label May 4, 2020
@carusogabriel carusogabriel requested a review from a team May 15, 2020 04:49
@carusogabriel
Copy link
Contributor Author

@doctrine/coding-standard-approvers Mind to review this one?

I'd like to get it in for v8.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of consistency, I'd prefer fn ($x). However, I also wouldn't object to a PR that enforces fn($x).

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as @alcaeus said.

@carusogabriel
Copy link
Contributor Author

@malarzm Would you agree with us merging this PR given the arguments above? :)

@malarzm
Copy link
Member

malarzm commented May 17, 2020

@carusogabriel I'm the only one objecting and there are already 4 approvals. Feel free to merge, I'm in minority anyway :)

@greg0ire greg0ire dismissed malarzm’s stale review May 17, 2020 09:49

We can't click the button if changes are requested

@greg0ire greg0ire merged commit f163ec4 into master May 17, 2020
@greg0ire greg0ire deleted the new-rule/arrow-functions-declaration branch May 17, 2020 09:49
@greg0ire
Copy link
Member

greg0ire commented May 17, 2020

Thanks @carusogabriel !

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

Successfully merging this pull request may close these issues.

7 participants