-
-
Notifications
You must be signed in to change notification settings - Fork 588
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
feat: support unions in arrays #1552
base: master
Are you sure you want to change the base?
Conversation
simPod
commented
Jul 19, 2024
•
edited
Loading
edited
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
Doc updated | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #... |
License | MIT |
- The changes to the @type lexer/parser may support the simplest array<A|B> test case, but I can think of some edge cases that would cause that type parsing to fail. The @type typer lexer/parser currently has no union support, so this adds support for array<A|B> but there's no support for just A|B.
- The only tests seem to be of the docblockparser (which is good), but no tests of any serialization/deserialization, no tests of the Type annotation lexer changes. + @simPod I think we need also E2E test of deserialisation - I think it should already work for simple types :)
- What is the expected behaviour of A|B[] in a docblock type?
8ad44a6
to
00bfa52
Compare
@idbentley can you help with code review, please? :) |
@simPod I think we need also E2E test of deserialisation - I think it should already work for simple types :) |
My main feedback is that this PR seems rather incomplete. The changes to the The only tests seem to be of the docblockparser (which is good), but no tests of any serialization/deserialization, no tests of the Type annotation lexer changes. What is the expected behaviour of Overall, I think this is a great feature, but needs some extra help. |
This is indeed rather incomplete. I'm now stuck with DeserializationGraphNavigator that does not know how to handle unions. |
'params' => [ | ||
['name' => 'int', 'params' => []], | ||
['name' => Product::class, 'params' => []], | ||
['name' => Vehicle::class, 'params' => []], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simPod It seems like array<int, string>
and array<int|string>
will create exactly the same metadata 😓 I would consider using Union
pseudo type same as for the unions, so it will be handled by the Union Handler :D