-
Notifications
You must be signed in to change notification settings - Fork 452
Move most HTML related interfaces to KDL #2155
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
I have rebased it @saschanaz |
src/build/patches.ts
Outdated
...optionalMember("optional", "boolean", child.properties?.optional), | ||
...optionalMember("overrideType", "string", child.properties?.overrideType), | ||
...optionalMember("type", "string", child.properties?.type), | ||
...optionalMember("nullable", "boolean", child.properties?.nullable), |
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.
nullable
belongs to type rather than property itself, so I think this should go to type node (and thus use type node in the patch file too)
I have updated it to use the type and also refactored the code @saschanaz |
Can you review this @saschanaz ? |
}; | ||
} | ||
|
||
function findTypeNode(children: Node[], context: string): Node | undefined { |
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.
Why the split? I don't find this easier to understand. The code is longer now while the logic is still same, right?
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.
This is actually shorter @saschanaz, since we now use it for both the method and the property. I factored it out into its own function to keep things DRY.
#2053
NOTE: I didn't migrate methods, because there is still some missing pieces in the parser