-
Notifications
You must be signed in to change notification settings - Fork 26
feat: json, json-all generators #287
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
❌ 14 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
@flakey5 is this still in progress? Need any help? |
|
Still in progress just haven't been committing, I do wanna get this ready for review within the next coming weeks though |
a4cd425 to
d711fee
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2d10df1 to
e4dbbe4
Compare
6bfc555 to
1bcf93d
Compare
|
Bulk of this is done, with only two main things left:
I'm leaving this as a draft until those are done, but the rest is ready for review |
avivkeller
left a comment
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.
the rest is ready for review
I've left a first round of reviews. Thank you so much for this effort!
ee1684d to
b37daee
Compare
b37daee to
8ebb3d6
Compare
|
Marking ready for review, should be all there minus #287 (comment) |
avivkeller
left a comment
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.
More comments :-)
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
1cf5261 to
0ec318d
Compare
|
|
||
| return { | ||
| $schema: `${BASE_URL}docs/${version}/api/${SCHEMA_FILENAME}`, | ||
| source: head.api_doc_source, |
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.
Currently this is something like doc/api/buffer.md, I wonder if it'd be more useful to give an actual link to the source file like https://nodejs.org/docs/v25.0.0/api/buffer.md
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Co-authored-by: Aviv Keller <me@aviv.sh>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Co-authored-by: Aviv Keller <me@aviv.sh>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
|
(Re-requesting my review as a reminder to myself to review this later today) |
avivkeller
left a comment
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.
Keep up the good work!
| if (typeof index !== 'number') { | ||
| index = Number(index); | ||
| } |
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.
When will this not be a number?
| if (entry.changes.length > 0) { | ||
| section.changes = entry.changes.map(change => ({ | ||
| description: change.description, | ||
| prUrl: change['pr-url'], | ||
| version: enforceArray(change.version), | ||
| })); | ||
| } | ||
|
|
||
| if (entry.added_in) { | ||
| section['@since'] = enforceArray(entry.added_in); | ||
| } | ||
|
|
||
| if (entry.n_api_version) { | ||
| section.napiVersion = enforceArray(entry.n_api_version); | ||
| } | ||
|
|
||
| if (entry.removed_in) { | ||
| section.removedIn = enforceArray(entry.removed_in); | ||
| } | ||
|
|
||
| if (entry.deprecated_in) { | ||
| section['@deprecated'] = enforceArray(entry.deprecated_in); | ||
| } |
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 should get in the habit of always supplying these props, even if they are empty/undefined
wdyt @ovflowd
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.
You mispelled my @ hahaha, but I agree :)
| } else { | ||
| // Put static methods in `staticMethods` property and non-static methods | ||
| // in the `methods` property | ||
| property = entry.heading.data.text.startsWith('Static method:') |
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 picked up by the classMethod RegEx, right? Do we need to check it here as well?
|
Imma gonna review this in a few (hopefully this weekend!) |
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't we git ignore this file?
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.
-1 on this, these only change when there's a change in the schema which shouldn't be that frequent. Adding this to the git ignore would just add another step to anyone wanting to contribute to this generator (and also idk if a jsdoc type ref to a non-existent file would throw a linter error or not)
Closes #214
TODO: