-
Notifications
You must be signed in to change notification settings - Fork 443
KDL Parser #2064
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?
KDL Parser #2064
Conversation
I think we'll need a minimal amount of migration in this PR, to confirm the code works 👀 |
It is just the same code as in #2056 |
That's in a different PR. It should be in the same PR so that it's easy to see the CI works on the PR, as the code can change during the review pingpong. |
I will add it today |
I have updated it @saschanaz |
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
I have updated it @saschanaz |
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.
Mostly looks nice to me! Some smaller nits:
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.
Looks cool. Thanks for the work! Just some last nits:
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Co-authored-by: Kagami Sascha Rosylight <[email protected]>
Your are welcome @saschanaz Thank you for taking the time to review this PR. |
* feat: FontDisplay * rename * feat: InsertPosition * feat: RTCStatsIceCandidatePairState * feat: GlobalCompositeOperation * PermissionName * - * Delete inputfiles/patches/imageOrientation.kdl
I have migrated more enums. Can you quickly check it again @saschanaz? |
Err I thought we'll do that in a separate PR |
(And I know it's an existing convention to use camelCase in file names, for new files I prefer hyphenated names as done in src/build/) |
(But perhaps @jakebailey would want consistency with the core typescript repository which also uses camel case) |
If you want i can make it a separate PR.
But all the input files are camelCased. If you want, I can create another PR to convert everything to be hyphenated even though I prefer camelCased. |
Let's do that |
Of course, my personal preference is not to have dashes in filenames like this, no. As for the PR as a whole, I'm not sure I understand the benefit of KDL specifically in this instance; can't the splitting be done without doing that? Pretty surprised by the repetition of the word "value" over and over. |
Technically we can skip enum Enum {
"foo"
"bar"
baz // Actually even the quote can be skipped
baz-baz // Because this is legal
} But this way we won't be able to tell the child node type anymore, as we treat the type as random input string. I was thinking about a chance to add another child node, but perhaps that won't happen and we can do it simpler. |
Ok... 🥺 |
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.
back to r- to roll back the additional enums. And we should also try the shorter form without value
per @jakebailey's feedback.
This reverts commit 2552032.
I have reverted it @saschanaz |
1 similar comment
I have reverted it @saschanaz |
Should we try the shorter form mentioned in #2064 (comment) ? (we can speed it up if you open another PR to my fork, if you want!) |
Sure, I will open another PR. Because I am very busy |
@jakebailey is it better now? 👀 (I wonder it's better to always quote or always not quote. It has no difference in the parse result though.) |
(LGTM from me) |
Sorry @saschanaz, you don't have access to these files: |
This PR is a small part of #2053.
It introduces the kdljs dependency and adds a parser utility that converts KDL text into the specific JSON structure we want.
Changes
Motivation
Part of the work to address #2053, where we need to handle KDL-formatted data in our system.
Notes