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

feat: support unions in arrays #1552

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jul 19, 2024

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT

@simPod simPod force-pushed the union-arrays branch 3 times, most recently from 8ad44a6 to 00bfa52 Compare July 19, 2024 10:20
@scyzoryck
Copy link
Collaborator

@idbentley can you help with code review, please? :)

@scyzoryck
Copy link
Collaborator

@simPod I think we need also E2E test of deserialisation - I think it should already work for simple types :)

@idbentley
Copy link
Contributor

My main feedback is that this PR seems rather incomplete.

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.

What is the expected behaviour of A|B[] in a docblock type?

Overall, I think this is a great feature, but needs some extra help.

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.

3 participants