Closed theodorejb closed 1 year ago
I quite hate the extra space, so if we can change the rule to not have it (the currently published version doesn't mention it at all, so nothing is finalized yet) I am fully on board. Hell, I'd get rid of the space on long-closures too, if we could. 😄
Independently of personal tastes, this introduces an inconsistency in the standard with long closures. As this is supposed to be a coding style standard, consistency is key, so I'm not sure this is a good idea....
@jrfnl Arrow functions have a unique syntax from full closures, aimed at conciseness, so I don't think it's inconsistent for them to have their own spacing rule.
Is there anything else I can do to move this forward?
No, I give pull requests that change the spec two full weeks in case anyone wants to weigh in.
On Mon, Nov 21, 2022 at 1:21 PM Theodore Brown @.***> wrote:
Is there anything else I can do to move this forward?
— Reply to this email directly, view it on GitHub https://github.com/php-fig/per-coding-style/pull/53#issuecomment-1322660038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHV6O6GGDSPYXOQ423HKQDWJPRT7ANCNFSM6AAAAAAR4VZLDQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Note: I also checked the PHP Prettier plugin and found that it, like PhpStorm, formats arrow functions without a space after the fn
keyword. Example.
Looks good to me, one thing that pops out at me is the following is incorrect:
The
fn
keyword MUST be preceded by a space
for example consider something like https://3v4l.org/SRSVo:
foo(fn(string $baz) => "Input: $baz");
// vs
foo( fn(string $baz) => "Input: $baz");
but this can be sorted out in a separate PR
Can we actually make some research on how arrow functions are used in the wild? PSR-12 team conducted a survey, gathering votes from 142 people including 17 project representatives. For this PR, I can see 4 people, including one being against this change.
requiring a space after the
fn
keyword (...) doesn't align with general usage
Sure, except for completely niche projects like Symfony, Laravel, PHPUnit, Composer, Magento, Doctrine, PHPStan, Flysystem, Laminas, Phalcon, PHP-CS-Fixer, Rector, Spatie, phpDocumentor... Also, I did a quick code search on GitHub:
requiring a space after the
fn
keyword (...) doesn't align with documentation
You mean PHP documentation? It doesn't seem to care about any coding style in the slightest (screenshot from https://www.php.net/manual/en/functions.anonymous.php).
PHP-CS-Fixer was an outlier which incorrectly enforced spacing after the
fn
keywordThis is also the default formatting enforced by PhpStorm.
I also checked the PHP Prettier plugin and found that it, like PhpStorm, formats arrow functions without a space after the fn keyword.
So defaults in PHP-CS-Fixer, which doesn't match your taste, are just incorrect, but bringing up defaults in PhpStorm and Prettier is a valid point in discussion?
Per the accepted Arrow Functions RFC and per its author, short closures were intended to be written without a space after the fn keyword.
That's actually the only argument that makes sense to me. However, the RFC author did allow a space after the fn
, I guess in order to let the users decide how they would like to use it, and it seems to me that many of them chose to go with the additional space.
Sure, except for completely niche projects like Symfony, Laravel, PHPUnit, Composer, Magento, Doctrine, PHPStan, Flysystem, Laminas, Phalcon, PHP-CS-Fixer, Rector, Spatie, phpDocumentor...
Out of that list, Symfony, PHPUnit, Composer Packagist, Magento, Flysystem, Phalcon, PHP-CS-Fixer, and Spatie are all using PHP-CS-Fixer, which until the latest release (v3.13) didn't allow differentiating between function
and fn
for formatting. So it seems unlikely that these projects chose to use an extra space; they simply didn't have the option to choose something different for fn
with PHP-CS-Fixer.
Another project in your list (Laminas) does not appear to enforce an arrow function spacing rule - some fn
keywords are succeeded by a space, but the majority are not.
Other well-known projects such as Psalm enforce having no space after the fn
keyword.
You mean PHP documentation? It doesn't seem to care about any coding style in the slightest (screenshot from https://www.php.net/manual/en/functions.anonymous.php).
That screenshot isn't related to arrow functions (I did submit a PR to fix that inconsistency, though). The official Arrow Functions documentation examples do use consistent formatting which matches the accepted RFC.
So defaults in PHP-CS-Fixer, which doesn't match your taste, are just incorrect, but bringing up defaults in PhpStorm and Prettier is a valid point in discussion?
My point was that PHP-CS-Fixer's defaults didn't match other popular tools. Now that PHP-CS-Fixer allows configuring spacing for fn
separately, you can already find projects that use it starting to opt into enforcing no space after fn
.
Since there hasn't yet been a released PSR standard for fn
spacing, it's natural that various projects have been using different styles. I have tried to avoid making an argument for one style or another based on my personal taste.
To summarize, the following arguments could be used to favor a space after fn
:
On the other hand, the following points favor no space after fn
:
fn
supports this goal.fn
. This is the formatting consistently used in the accepted RFC, as well as the official PHP documentation for arrow functions.fn
. I don't have access to direct numbers, but my hunch is that PhpStorm is used with far more projects than PHP-CS-Fixer is.A bunch of large open-source projects use this formatting, since PHP-CS-Fixer defaulted to it and didn't use to allow formatting arrow functions separately from full closures.
it seems unlikely that these projects chose to use an extra space; they simply didn't have the option to choose something different for
fn
with PHP-CS-Fixer.
They did have the option, it's as simple as 'function_declaration' => false
. They aren't forced to use neither this rule nor the PHP-CS-Fixer itself, yet they've chosen to do so.
Now that PHP-CS-Fixer allows configuring spacing for
fn
separately, you can already find projects that use it starting to opt into enforcing no space afterfn
.
This option was released almost a month ago, and none of the big projects I brought up changed it so far.
Why do you think that all of these projects have been forced to use this spacing by PHP-CS-Fixer? What makes you sure that any of these projects wants to change that? All of them were free to format their code however they want. They've chosen. They can change it whenever they want, yet they don't.
You mean PHP documentation? It doesn't seem to care about any coding style in the slightest
That screenshot isn't related to arrow functions
It wasn't supposed to be related to arrow functions, it was supposed to show my point: PHP documentation doesn't care about any coding style, so bringing it up in a discussion about coding style makes no sense.
My point was that PHP-CS-Fixer's defaults didn't match other popular tools.
Other popular PHP formatters such the one built into PhpStorm, and Prettier's PHP plugin default to no space after
fn
. I don't have access to direct numbers, but my hunch is that PhpStorm is used with far more projects than PHP-CS-Fixer is.
PhpStorm may be used in far more projects, but not as a formatter. PhpStorm can't even be used to enforce consistent formatting, as you can't use it in your CI. And for Prettier we have numbers: 1.5k stars for Prettier, 11.5k stars for PHP-CS-Fixer. So my question remains: why do you call defaults in PHP-CS-Fixer “incorrect”, but bring up defaults in PhpStorm and Prettier is a valid point in discussion?
The primary goal of arrow functions is to avoid the verbosity of full closures. Not having a space after
fn
supports this goal.
verbose: containing more words than necessary (Merriam-Webster) verbose: using or containing more words than are necessary (Cambridge)
Sorry for taking apart your words, but I think there's a point there: having more whitespace is something different than having more words, and it makes code easier to read for some people. You don't need to add any spaces around binary operators, after the type colon, or between function arguments, yet you do for some reason. You could probably even format most of your PHP files to be one line to reduce “verbosity”.
Arrow functions were intended to be written without a space after
fn
. This is the formatting consistently used in the accepted RFC, as well as the official PHP documentation for arrow functions.
I agree with the point about the RFC. The documentation argument doesn't make any sense though, for the reason stated above. There is anything but coding style consistency in PHP documentation.
Another project in your list (Laminas) does not appear to enforce an arrow function spacing rule Other well-known projects such as Psalm enforce having no space after the
fn
keyword.
You're right, my bad with Laminas. Also, I'm not saying that nobody prefers this style, Nette seems to use it too.
My point with bringing up these projects is that it's just false that fn(
is a “general usage”. Many major projects do use fn (
, and code search on GitHub reveals that it's even more popular than fn(
.
fn (
fn (
fn (
fn (
fn (
than fn(
found with a simple GitHub searchThis option was released almost a month ago, and none of the big projects I brought up changed it so far.
You vastly under-estimate the effort involved in changing coding standards on a large project. IME, it can take months just to agree to do it, to say nothing of actually making the change.
@jrmajor is correct that we should be informed by research, however. Which he has, graciously, done for us. If we take the screenshot from GitHub at face value, it tells us that while fn (
is more common, it's not by much. It's around 53% fn (
, 47% fn(
. I'd call that close enough to an even split. That means no matter what we do, "around half" of existing code will not already be in compliance with PER-CS 1.1.
It's also a valid observation that it seems very likely that many/most of those projects didn't make an active, deliberate decision. They just turned on php-cs-fixer, or Prettier, or whatever tool and left it at its defaults, or made only one or two changes. That's true for both styles, probably.
So if we make a statement at all, it's going to disrupt about the same amount of code either way. And odds are, those people will upgrade their formatting tool at some point in the future, it will say "now using PER-CS 1.1", and flag a bunch of things. Users will roll their eyes, push "yes", and move on with their lives.
So our decision should be based on internal consistency within the spec, and on what is most ergonomic (which is admittedly hard to get any objective data about in this case). The popularity argument is basically a wash either way.
It's also a valid observation that it seems very likely that many/most of those projects didn't make an active, deliberate decision. They just turned on php-cs-fixer, or Prettier, or whatever tool and left it at its defaults, or made only one or two changes. That's true for both styles, probably.
You vastly under-estimate the effort involved in changing coding standards on a large project. IME, it can take months just to agree to do it, to say nothing of actually making the change.
Even if it wasn't as deliberate decision, which I doubt, there is nothing suggesting that these projects want to change that. And there's high chance they won't, for the exact reason you provided.
If we take the screenshot from GitHub at face value, it tells us that while
fn (
is more common, it's not by much. It's around 53%fn (
, 47%fn(
. I'd call that close enough to an even split. That means no matter what we do, "around half" of existing code will not already be in compliance with PER-CS 1.1.So if we make a statement at all, it's going to disrupt about the same amount of code either way.
The popularity argument is basically a wash either way.
I find the way you rounded these percentages unfair. Anyway, I've taken this screenshot only to show that even the simplest research one can do reveals that the claim about the “general usage” is false.
For more accurate results, I've analysed 1000 most popular Composer packages using https://github.com/nikic/popular-package-analysis and this script. Here are the results:
total files analysed: 109516
total with a space: 1619, 72%
total without a space: 631, 28%
Which is in line with the fact that most popular tools in all categories I could think of also use fn (
.
In the end, we can all live with whatever ends up being included in PER CS. My main complaint isn't as much about the fn(
, as it is about the process that led to this PR being merged. I believe that most arguments brought up made no sense, as justified in previous comments. The only argument related to the real usage, which is most important IMO, was just calling it “the general usage” with no research provided to support this claim, and the more research I do, the more false it turns out to be.
Now the function began to look like everything was thrown into one pile...
fn(string $foo) => $foo;
fn(string $bar) => $bar;
It's ugly and inconvenient. Also difficult to read.
The option with a space looks much better and more elegant:
fn (string $foo) => $foo;
fn (string $bar) => $bar;
But, since we removed the spaces from the arrow functions, why not remove the spaces in other places as well? For example:
(string)$foo
(int)$bar
function(string $foo) {}
Per the accepted Arrow Functions RFC and per its author, short closures were intended to be written without a space after the
fn
keyword. This is also the default formatting enforced by PhpStorm.PHP-CS-Fixer was an outlier which incorrectly enforced spacing after the
fn
keyword using the same setting as for thefunction
keyword. This was a bug and has been corrected with the addition of a separateclosure_fn_spacing
setting (though this setting still defaults toone
for backwards compatibility - the plan was to change it to enforce no space in the next major 4.0 version). But the current PER coding style spec may get in the way of this plan.Since the primary goal of arrow functions is to avoid the verbosity of anonymous functions, requiring a space after the
fn
keyword is a step backwards, and also doesn't align with general usage promoted by PhpStorm as well as the accepted PHP RFC and documentation.The section on Short Closures was not added until the 17th of July by https://github.com/php-fig/per-coding-style/pull/17, which was after the 1.0.0 spec was approved/released on June 9, so I'm hoping it's not too late to correct it.