Support multiple types & update toJSON - #2304
Conversation
|
Thanks for the PR! This section of the codebase is owned by Kagami Sascha Rosylight (@saschanaz) - if they write a comment saying "LGTM" then it will be merged. |
|
What do you think Lucas Garron (@lgarron) |
| function handleMultipleTypes( | ||
| typeNodes: Node[], | ||
| returns?: Value, | ||
| ): DeepPartial<Typed> | DeepPartial<Typed>[] | undefined { |
There was a problem hiding this comment.
It's DeepPartial<Typed> | undefined, no? It never returns an array.
And this function should be called handleTyped and the singular version should be called handleSingleTypeNode or something.
| ...optionalMember("overrideType", "string", child.properties?.overrideType), | ||
| ...(typeNode | ||
| ? handleTyped(typeNode) | ||
| ? handleSingleTypeNode(typeNode) |
There was a problem hiding this comment.
Why? properties can have union types too. handleSingleTypeNode should not be called outside handleTyped.
| const subType = | ||
| type.children.length > 0 ? handleTyped(type.children[0]) : undefined; | ||
| type.children.length > 0 | ||
| ? handleSingleTypeNode(type.children[0]) |
There was a problem hiding this comment.
And subtypes can be union too
|
Done |
|
|
||
| function handleTyped( | ||
| typeNodes: Node[], | ||
| returns?: Value, |
There was a problem hiding this comment.
returns? you mean property?
There was a problem hiding this comment.
Yes, I forgot to include it i will fix it
There was a problem hiding this comment.
You still forgot this
There was a problem hiding this comment.
I have renamed it
|
Better? |
|
Is there any thing missing here Kagami Sascha Rosylight (@saschanaz) |
|
|
||
| interface AuthenticationExtensionsClientOutputsJSON { | ||
| appid?: boolean; | ||
| appidExclude?: boolean; |
There was a problem hiding this comment.
Looks like this has no implementation, otherwise seems good
|
LGTM, thanks! |
|
Merging because Kagami Sascha Rosylight (@saschanaz) is a code-owner of all the changes - thanks! |
2 participants
fixes #2114