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

New PHP syntax worth considering #3

Closed
22 of 25 tasks
KorvinSzanto opened this issue Jan 15, 2022 · 17 comments
Closed
22 of 25 tasks

New PHP syntax worth considering #3

KorvinSzanto opened this issue Jan 15, 2022 · 17 comments
Labels

Comments

@KorvinSzanto
Copy link
Contributor

KorvinSzanto commented Jan 15, 2022

I've gone through the PHP versions since 7.3 which was the most recently released when PSR-12 was accepted and gathered a list of syntax that we may want to cover in this PER. This list is likely incomplete so please add a comment if there's more I should add.

PHP 7.3: https://www.php.net/manual/en/migration73.php

PHP 7.4: https://www.php.net/manual/en/migration74.php

PHP 8.0: https://www.php.net/manual/en/migration80.php

PHP 8.1: https://www.php.net/manual/en/migration81.php

PHP 8.2: https://github.com/php/php-src/milestone/4

Extra potential things to cover:

  • Visibility keyword is not required when specifying readonly: public function __construct(readonly $foo)
@Girgias
Copy link

Girgias commented Jan 15, 2022

__serialize and __unserialize vs using Serializable

Serializable is deprecated (see https://wiki.php.net/rfc/phase_out_serializable) and is meant as a compatibility layer for old implementations.

FILTER_VALIDATE_BOOL over FILTER_VALIDATE_BOOLEAN

FILTER_VALIDATE_BOOL was added as it is the canonical type name, therefore this one should be recommended IMHO

For composite types, no spaces IMHO, especially when mixing union and intersection types (soon inc.) having loads of spaces is going to be detrimental to readability IMHO.

@KorvinSzanto
Copy link
Contributor Author

KorvinSzanto commented Jan 15, 2022

Serializable is deprecated (see https://wiki.php.net/rfc/phase_out_serializable) and is meant as a compatibility layer for old implementations.

I wanted to include anything not removed just in case

FILTER_VALIDATE_BOOL was added as it is the canonical type name, therefore this one should be recommended IMHO

👍 We already require folks use bool over boolean so this makes sense.

For composite types, no spaces IMHO, especially when mixing union and intersection types (soon inc.) having loads of spaces is going to be detrimental to readability IMHO.

I'm not so sure on this one, PSR-12 currently prefers spaces around other binary operators and other languages that have similar syntax also tend to prefer the spaces. 1 2 3 I agree these declarations will get harder to read as they get longer especially since we don't have a great way to make larger union types portable like type FooBarBazType = Foo | Bar | Baz;. That said, I think we're going to have that issue whether we require spaces or not so we will probably need think about multiline declarations with something like:

function foo(): 
    null |
    SomeReallyLongClassName |
    SomeReallyLongOtherClassNameInterface |
    SomeReallyLongOtherOtherClassNameInterface
{
    return 1;
}

Footnotes

  1. https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#unions-with-common-fields

  2. https://flow.org/en/docs/types/intersections/

  3. https://docs.python.org/3/library/typing.html#typing.Union

@Jean85
Copy link
Member

Jean85 commented Jan 18, 2022

In regard to multiline declaration, IMHO we should suggest to put the operators at the start of the line, so it makes immediately clear that there's a continued declaration (or operation, in other contexts like with || and &&):

function foo(): 
    null
    | SomeReallyLongClassName
    | SomeReallyLongOtherClassNameInterface
    | SomeReallyLongOtherOtherClassNameInterface
{
    return 1;
}

@Crell
Copy link
Collaborator

Crell commented Apr 29, 2022

From what I've seen, union types are generally written without spaces right now. At least that's what TYPO3 has been doing as we add types for v12.

IF going multiline, I agree the | should be at the start, as is typical for other similar constructs.

@ricardoboss
Copy link

Looking at Typescript, union and intersection type declarations include spaces around operators (see https://github.com/microsoft/TypeScript/blob/main/src/lib/es5.d.ts#L174 for example).

@Crell
Copy link
Collaborator

Crell commented Jun 1, 2022

PR for short closures: #17

@Crell
Copy link
Collaborator

Crell commented Jun 1, 2022

PR for trailing commas: #18

@Crell
Copy link
Collaborator

Crell commented Jun 1, 2022

PR for typed properties and readonly: #19

@Crell
Copy link
Collaborator

Crell commented Jul 8, 2022

PR for attributes: #26

@Crell
Copy link
Collaborator

Crell commented Jul 8, 2022

Things I think we can be silent on:

  • Numeric literal separator: Is there a guideline here to use?
  • __serialize vs Serializable: The latter is deprecated anyway, so I don't know that we have much to add beyond the language itself.
  • Throw as an expression: I really don't know what we'd even say here.
  • new in parameter defaults: Again, I'm not sure there's anything to standardize here that isn't already covered by other statements. (Eg, space on either side of the =.)

@Crell
Copy link
Collaborator

Crell commented Jul 8, 2022

PR for FCC: #27

@TimWolla
Copy link
Contributor

TimWolla commented Jul 8, 2022

Numeric literal separator: Is there a guideline here to use?

I don't think there is a generally applicable guideline other than “yes, please make use of it”. The “correct” grouping depends on a context:

const ONE_EURO_IN_CENTS = 1_00;
const THOUSAND_EUROS_IN_CENTS = 1_000_00;
const HUNDRED_THOUSAND = 100_000;
const ONE_MILLION = 1_000_000;

@Crell
Copy link
Collaborator

Crell commented Sep 12, 2022

Handles readonly classes: #41

@Crell
Copy link
Collaborator

Crell commented Sep 12, 2022

I'm going to argue that CPP is already adequately covered by existing rules, so nothing new needs to be said.

Named arguments are here: #42

@Crell
Copy link
Collaborator

Crell commented Sep 12, 2022

Nested ternaries: #43

@Crell
Copy link
Collaborator

Crell commented Sep 12, 2022

OK, I have gone through and struck out any changes that I believe require no action, either because they're not worth it or existing rules are already sufficient. Korvin, if you disagree with any of those go ahead and un-strike them.

Everything else now has an active PR. Discussion on those PRs. 😄

@Crell
Copy link
Collaborator

Crell commented Jan 13, 2023

I'll call this done at this point.

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

No branches or pull requests

7 participants