Conversation
| function veryComplex( | ||
| array | ||
| |(ArrayAccess&Traversable) | ||
| |(Traversable&Countable) $input): ArrayAccess&Traversable |
There was a problem hiding this comment.
| |(Traversable&Countable) $input): ArrayAccess&Traversable | |
| |(Traversable&Countable) $input | |
| ): ArrayAccess&Traversable |
There was a problem hiding this comment.
Hm. Not sure here. Does this context also force multilining the return part? If so, that should be said explicitly somewhere.
There was a problem hiding this comment.
Yeah doing this would split the argument list over multiple lines and would make "argument list is split across multiple lines" rules apply here. I get that this is different than actually splitting each argument onto their own lines but I don't think that distinction matters at all when it comes to readability and consistency.
KorvinSzanto
left a comment
There was a problem hiding this comment.
Needs:
- Consistency with
catch()or an argument for not being consistent - Consistency with mutliline declaration requirements
|
I think the catch consistency question is a big one so I've opened a vote in discord for the WG to decide the way to move forward. |
|
A while back, I put out a Mastodon poll on this. Not exactly scientific, but probably the best we've got. https://phpc.social/@Crell/112496711448592596 That poll at least comes out 60/40 in favor of no-space. (Some interesting comments either way in the replies.) Symfony, doesn't make an explicit stance, but their examples also show no-space (link courtesy Ben Ramsey): https://symfony.com/doc/current/contributing/code/standards.html And the default configuration for php-cs-fixer also defaults to no-space: https://cs.symfony.com/doc/rules/whitespace/types_spaces.html#rule-sets Laravel doesn't say explicitly either, but its examples have no-space in compound types in the docblock: https://laravel.com/docs/11.x/contributions#coding-style Based on that, I am strongly leaning toward requiring no-space for compound types, as the PR currently does. And then probably changing union-catch to match. WG members (only): Yay 👍 or nay 👎, based on the above? |
I should note that the latter is surely causing the former, since Symfony uses PHP-CS-Fixer for its code (it even has a dedicated preset). |
|
No-space is used in Yii as well. |
|
Based on the above research and lack of objection to it, we will proceed with this PR (which I just rebased) unless I hear an articulated objection in the next week or so. @KorvinSzanto Please unblock the PR. 😄 |
|
Overall consistency is important, so I would expect the later revision of PER-CS to disallow spaces around "|" for catch statements. |
KorvinSzanto
left a comment
There was a problem hiding this comment.
I have to approve to remove my request for changes, but I believe this needs to be a 3.0 change given that we're modifying catch
| } | ||
| ``` | ||
|
|
||
| If one of the ORed conditions is `null`, it MUST be the last item in the list. |
There was a problem hiding this comment.
Why null must be the last? It's kinda inconsistent with ?X, since there null goes first.
There's likely to be discussion on this, but this PR gets us started. Discuss.