Handle Properties in kdl #2088
Handle Properties in kdl #2088
Conversation
…vents and properties from mixin nodes. Update `handleMixin` to utilize these new functions, improving code organization and clarity.
|
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. |
src/build/patches.ts
Outdated
| mixins[name] = { name, events: { event } }; | ||
|
|
||
| const event: Event[] = extractMixinEvents(node); | ||
| const property: Properties = extractMixinProperties(node); |
There was a problem hiding this comment.
I think we'd want to iterate on children rather than filtering, so that if there's an unknown child then it can throw. Just like how handleMixin is called from parseKDL.
There was a problem hiding this comment.
Is it now better @saschanaz, I have updated it :)
src/build/patches.ts
Outdated
| property[name] = { | ||
| name, | ||
| exposed: child.properties?.exposed as string, | ||
| }; |
There was a problem hiding this comment.
I think separate functions for each is better, as I'm sure this will become longer, even for property alone.
There was a problem hiding this comment.
I have created smaller functions @saschanaz, would you prefer smaller files?
There was a problem hiding this comment.
I think this wouldn't become super huge to justify extra modules, for now let's keep it this way and see how it goes.
src/build/patches.ts
Outdated
| * @param child The child node to handle. | ||
| * @param event The event array to update. | ||
| */ | ||
| function handleEventChild(child: Node, event: Event[], name: string) { |
There was a problem hiding this comment.
Every event will be child so no need to have Child in the name. Same for property.
There was a problem hiding this comment.
I have renamed it @saschanaz
src/build/patches.ts
Outdated
| * @param event The event array to update. | ||
| */ | ||
| function handleEvent(child: Node, event: Event[], name: string) { | ||
| event.push({ |
There was a problem hiding this comment.
Can we return Event here and do event.push in the caller? Like event.push(handleEvent(child)).
Also probably better to get the name in each function, as I think not all members will have a name, e.g. constructor.
There was a problem hiding this comment.
I have updated it for events and properties
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
Co-authored-by: Kagami Sascha Rosylight <saschanaz@outlook.com>
|
Thank you @saschanaz, |
| } | ||
| } | ||
|
|
||
| mixins[name] = { name, events: { event }, properties: { property } }; |
There was a problem hiding this comment.
Would be nice to return a value as handleProperty now does. But as handleEnum already does this, this can be done in a separate PR.
|
LGTM |
|
Merging because @saschanaz is a code-owner of all the changes - thanks! |
reference #2053
I've added support for handling properties and refactored the mixin handling into smaller, more manageable functions.