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

[PHP 8.1] Add Enums #758

Merged
merged 9 commits into from
Apr 25, 2021
Merged

[PHP 8.1] Add Enums #758

merged 9 commits into from
Apr 25, 2021

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented Feb 6, 2021

Closes #757

enum Suit {
  case Hearts;
  case Diamonds;
  case Clubs;
  case Spades;
}

@TomasVotruba TomasVotruba changed the title php81 enums [WIP] [PHP 8.1] Add Enums Feb 6, 2021
@szepeviktor
Copy link
Contributor

[WIP] is really a Draft PR on GitHub :)

kép

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Feb 6, 2021

I know, thanks :) I've seen more people forgot to switch that than to make it useful.
This is rather comment for me to keep me going :D

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Feb 6, 2021

@nikic Hi, I made first draft, but got stuck on unclear error probably related to enum name grammar. I'm lost about why enum is not taken as keyword.

Could you review the PR so far? Thank you

@TomasVotruba TomasVotruba changed the title [WIP] [PHP 8.1] Add Enums [PHP 8.1] Add Enums Feb 6, 2021
@szepeviktor
Copy link
Contributor

Hi, I made first draft, but got stuck on

Hi, I made first [WIP], but got stuck on ... 😃

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 20, 2021

@nikic Hi, since Enums are now accepted and merged in PHP core, could you give me feedback on this?
Thank you 👍

@nikic
Copy link
Owner

nikic commented Mar 20, 2021

Can you please rebase over a8223f2? The enum keyword needs slightly special handling.

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Mar 21, 2021

Thanks! I've rebased and grammar works perfectly 👍


I'm adding more tests case and there is weird one, where string is not interpreted as Identifier, but as Name. I have tried to run test update, but it didn't change anything:

php test/updateTests.php

Any how to fix it?


Otherwise it's ready to review

@nikic
Copy link
Owner

nikic commented Mar 21, 2021

@TomasVotruba Possibly this is because you added enums to the PHP 5 grammar as well, where string is a class type? I would suggest to drop them from php5.y and mark tests as !!php7.

@TomasVotruba TomasVotruba force-pushed the php81-enums branch 2 times, most recently from 9bb9e0a to 3d0f68d Compare March 21, 2021 10:15
@TomasVotruba
Copy link
Contributor Author

I've removed enum from php5.y grammer and it looks good 👍

lib/PhpParser/Node/Stmt/EnumCase.php Outdated Show resolved Hide resolved
lib/PhpParser/Node/Stmt/EnumCase.php Outdated Show resolved Hide resolved
lib/PhpParser/Node/Stmt/Enum_.php Outdated Show resolved Hide resolved
lib/PhpParser/ParserAbstract.php Outdated Show resolved Hide resolved
lib/PhpParser/PrettyPrinter/Standard.php Show resolved Hide resolved
test/code/formatPreservation/enum.test Show resolved Hide resolved
@TomasVotruba TomasVotruba force-pushed the php81-enums branch 3 times, most recently from 5cf45d2 to ccf27f1 Compare March 21, 2021 22:52
@TomasVotruba
Copy link
Contributor Author

Ready to review ✔️

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Apr 5, 2021

I've processed your comments and CI is passing with both format preserving and normal tests 👍
Let me know if there is something missing, so I can finish it.

@nikic nikic merged commit f68e1a4 into nikic:master Apr 25, 2021
@TomasVotruba TomasVotruba deleted the php81-enums branch April 25, 2021 19:27
@TomasVotruba
Copy link
Contributor Author

Thanks 🥳

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

Successfully merging this pull request may close these issues.

[PHP 8.1] Add enumarations
3 participants