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.

Sign up for GitHub

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

Jump to bottom

[3.x] feat: add support for DNF types #236

Merged
merged 2 commits into from
Sep 14, 2022
Merged

[3.x] feat: add support for DNF types #236

merged 2 commits into from
Sep 14, 2022

Conversation

nhedger
Copy link
Contributor

@nhedger nhedger commented Sep 8, 2022 •
edited
Loading

This PR adds support for Disjunctive Normal Form Types (DNF types) when using PHP 8.2.

Overview of the changes

  • Removes the previous guard that would only allow ReflectionNamedType to be analyzed.
  • Adds two new internal functions matchesType and matchesAllTypes to extract common functionality.
  • Adds tests + supporting fixtures

Please let me know if you think that I missed something.

Closes #235

@nhedger nhedger changed the title feat: add support for DNF types [3.x] feat: add support for DNF types Sep 8, 2022
@nhedger nhedger marked this pull request as ready for review September 8, 2022 17:25
SimonFrings
SimonFrings approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

@nhedger Looks good to me 👍

clue
clue reviewed Sep 14, 2022
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@nhedger Thank you very much for these high quality changes! Functionally, this LGTM and very much appreciate the added tests.

I've added some minor remarks regarding internal functions only, WDYT?

src/functions.php Outdated Show resolved Hide resolved
src/functions.php Outdated Show resolved Hide resolved
@clue clue added this to the v3.0.0 milestone Sep 14, 2022
clue
clue approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@nhedger Thanks for the quick update, changes LGTM! Keep it up! 👍

@clue clue requested a review from WyriHaximus September 14, 2022 15:45
WyriHaximus
WyriHaximus approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for putting this together 👍

@WyriHaximus WyriHaximus merged commit ee59eae into reactphp:3.x Sep 14, 2022
@nhedger nhedger deleted the feat/dnf branch September 14, 2022 18:53
@clue
Copy link
Member

clue commented Sep 15, 2022

@nhedger Excellent work! Ticket #235 suggests applying the same changes also for 2.x, do you plan to backport this to the other branch or should anybody else take a look at this? 👍

@nhedger
Copy link
Contributor Author

nhedger commented Sep 15, 2022

I'll do it, probably before the end of the week.

@nhedger nhedger mentioned this pull request Sep 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WyriHaximus WyriHaximus WyriHaximus approved these changes

@clue clue clue approved these changes

@SimonFrings SimonFrings SimonFrings approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nhedger @clue @WyriHaximus @SimonFrings