-
Notifications
You must be signed in to change notification settings - Fork 5
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: legacy json generator #142
base: main
Are you sure you want to change the base?
Conversation
@flakey5 PTAL |
6e80d01
to
e2c8fdd
Compare
Currently, this PR is 1:1 with the original JSON (AFAIK), except for the following differences, which probably don't need to be fixed:
|
So, is it correct to assume that this is PR supersedes @flakey5's PR? Was this communicated with @flakey5? Are they fine with the continuation on this PR? Note that you could also have pushed your work on their PR if they were fine :) -- or at least you can once you join the team on GitHub I believe. I will review this once I get time 🙏 |
I would wait until we get the go-ahead from the other PR. My plan was to open this PR into that branch, but there was merge conflicts. If we don't get the go ahead, I suggest that @flakey5 use (I obviously don't want to take away from the other PR / cut off @flakey5's amazing work, and if we'd rather keep the discussion there, I am fine with that) |
BTW @RedYetiDev CodeQL complained about your code, that it might have a security vuln; Mind giving an eye? 👀 |
|
I was either traveling or getting over jet lag this weekend so I wasn't able to respond much to the comments made on #92, I would've appreciated a little more time to respond to the comments made but it's water under the bridge. Imo it makes sense to continue with this one since the commits are already here and it's pretty much done, still need to make sure the unresolved comments in #92 are addressed here however |
Sorry!! Thank you for understanding. Again, I'm happy to instead have this merged into your PR, I really don't want to take over your amazing work |
* | ||
* @param {import('../types.d.ts').Section} section | ||
*/ | ||
const parseList = section => { |
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 should be a Factory (even a different file) that allows you to handle the signature types for other types.
So that we have tiny methods (that implement an interface) instead of non-ending signatures
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.
What do you mean? Can you give a tiny example (sorry, it's not clicking in my head for me)
Thanks for the review! I'll check it out and update the code later today! |
I’ve attempted to resolve the review comments, when you get a chance, let me know if there are any other concerns. |
|
||
// Recursively parse nested options if a list exists | ||
const optionsNode = child.children.find(node => node.type === 'list'); | ||
if (optionsNode) { |
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.
Can you use &&=
(or something similar directly within the return statement?
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.
How so?
*/ | ||
const extractPattern = (text, pattern, key, current) => { | ||
const match = text.match(pattern)?.[1]?.trim().replace(/\.$/, ''); | ||
if (match) { |
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.
I believe this can be simplified
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.
I don't think it can? What do you have in mind?
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.
I rewrote it, but it's not simplified, LMK what you think
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.
I think we're almost there. Left a final round of comments. I'd also like to have some benchmarks, how long does the tooling take to run with all the doc files for only the legacy-json generator, and if long enough would you be able to attach a debugger and find slow paths?
(BTW, I wanted to clarify that this is fantastic work! I enormously appreciate your effort here!) |
And I appreciate your reviews! And @flakey5's initial PR! |
I'll look for slow paths. Right now (for
|
Closes #57
Closes #141
Closes #92
This is an extension of #92. This is a seperate PR due to a rebase that caused merge conflicts.
Description
See #92 for a description
Validation
Validate with
bash ./file.sh addons
,bash ./file.sh fs
, etc.Differences
These are the small differences, and likely do not need to be fixed.
textRaw
(I.E.[`something`][]
in the old parser is`something`
in the new one)meta
is always a property.Check List