feat: json, json-all generators #287
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
==========================================
+ Coverage 74.60% 82.42% +7.81%
==========================================
Files 112 140 +28
Lines 10700 15034 +4334
Branches 722 1027 +305
==========================================
+ Hits 7983 12392 +4409
+ Misses 2714 2637 -77
- Partials 3 5 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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 |
flakey5
force-pushed
the
flakey5/20240909/new-json-gen
branch
from
a4cd425 to
d711fee
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
flakey5
force-pushed
the
flakey5/20240909/new-json-gen
branch
from
2d10df1 to
e4dbbe4
Compare
flakey5
force-pushed
the
flakey5/20240909/new-json-gen
branch
from
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
avivkeller
left a comment
There was a problem hiding this comment.
the rest is ready for review
I've left a first round of reviews. Thank you so much for this effort!
There was a problem hiding this comment.
Do we need a seperate generator error?
There was a problem hiding this comment.
It exists just to give the general spot in the ast that an error happens to assist with debugging, can definitely be removed if unwanted
There was a problem hiding this comment.
@nodejs/web-infra im +1 on removing, WDYT?
src/generators/json/constants.mjs
Outdated
| 'use strict'; | ||
|
|
||
| // Grabs the default value if present | ||
| export const DEFAULT_EXPRESSION = /^(D|d)efault(s|):$/; |
There was a problem hiding this comment.
Let's re-use the DEFAULT_EXPRESSION constant from the legacy generator
There was a problem hiding this comment.
We can't since this is used for matching against the text in the ast which doesn't have the asterisks, while the one in the legacy generator is going off of the stringified version in textRaw which does
flakey5
force-pushed
the
flakey5/20240909/new-json-gen
branch
from
ee1684d to
b37daee
Compare
Closes #214 Signed-off-by: flakey5 <[email protected]>
flakey5
force-pushed
the
flakey5/20240909/new-json-gen
branch
from
b37daee to
8ebb3d6
Compare
|
Marking ready for review, should be all there minus #287 (comment) |
| input.forEach(section => { | ||
| const copiedSection = {}; | ||
|
|
||
| Object.keys(section).forEach(key => { | ||
| if (!propertiesToIgnore.includes(key)) { | ||
| copiedSection[key] = section[key]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
| input.forEach(section => { | |
| const copiedSection = {}; | |
| Object.keys(section).forEach(key => { | |
| if (!propertiesToIgnore.includes(key)) { | |
| copiedSection[key] = section[key]; | |
| } | |
| }); | |
| input.forEach({ ignoreThisKey, ...section } => { |
WDYT?
| switch (section.type) { | ||
| case 'module': | ||
| generatedValue.modules.push(copiedSection); | ||
| break; | ||
| case 'text': | ||
| generatedValue.text.push(copiedSection); | ||
| break; | ||
| default: | ||
| throw new TypeError(`unsupported root section type ${section.type}`); | ||
| } |
There was a problem hiding this comment.
| switch (section.type) { | |
| case 'module': | |
| generatedValue.modules.push(copiedSection); | |
| break; | |
| case 'text': | |
| generatedValue.text.push(copiedSection); | |
| break; | |
| default: | |
| throw new TypeError(`unsupported root section type ${section.type}`); | |
| } | |
| generatedValue[type === 'module' ? 'modules' : type].push(copiedSection) |
nit
src/generators/json-all/index.mjs
Outdated
| */ | ||
| async generate(input, { version, output }) { | ||
| const generatedValue = { | ||
| $schema: `${BASE_URL}/docs/${DOC_NODE_VERSION}/api/node-doc-all-schema.jsonc`, |
There was a problem hiding this comment.
Can we make a SCHEMA_URL constant?
There was a problem hiding this comment.
Not really since it intakes the version given by the generator options, same for the rest of the schema url instances
| * @param {string} version | ||
| */ | ||
| export function generateJsonSchema(version) { | ||
| const jsonSchemaUrl = `${BASE_URL}/docs/${version}/api/node-doc-schema.json`; |
| // Returns: {string} | ||
| // Returns {string} | ||
| // Returns: {string} bla bla bla | ||
| export const METHOD_RETURN_TYPE_EXTRACTOR = /^(?:R|r)eturns(:?) {(.*)}( .*)?$/; |
There was a problem hiding this comment.
I'd rather do a case insensitive search
There was a problem hiding this comment.
This isn't just checking if it starts with Returns: though, it's extracting the type information and possible description from it as well
| } | ||
|
|
||
| return { | ||
| $schema: `${BASE_URL}docs/${version}/api/node-doc-schema.json`, |
There was a problem hiding this comment.
@nodejs/web-infra im +1 on removing, WDYT?
Co-authored-by: Aviv Keller <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
| ); | ||
| } | ||
|
|
||
| if (rest[0]?.type === 'link') { |
There was a problem hiding this comment.
Can we have these specific extractors on dedicated function for ... extracting these? Also are the types here only link and "else"?
|
|
||
| parameter['@name'] = 'value'; | ||
|
|
||
| const { types, endingIndex } = parseTypeList(parameterAst.children); |
|
|
||
| break; | ||
| } | ||
| case 'text': { |
| // it | ||
| const { types, endingIndex } = parseTypeList(paragraph.children, 2); | ||
|
|
||
| if (types.length === 0) { |
| } | ||
| case 'link': { | ||
| const { types, endingIndex } = parseTypeList(paragraph.children, 1); | ||
| if (types.length === 0) { |
There was a problem hiding this comment.
Can we mhave this file output be in a different folder? Like a generated folder?
There was a problem hiding this comment.
Shouldn't this be automatically generated?
There was a problem hiding this comment.
The schema should be maintained by hand. It doesn't need to be in a standalone json file though, we could have it be a function like with the json-all's schema. Only reason it is standalone currently is to make it so we can have better intellisense/autocomplete in editors when modifying the schema
ovflowd
left a comment
ovflowd
left a comment
There was a problem hiding this comment.
Mostly LGTM, I think that we can dedup lots of the code, extract many of the utility functions within parsing things (such as event, property) into dedicated tiny functions that do one thing and then unit test them.
Makes the parent functions simpler, easier to read the code and understand what pieces does what, reusability and unit testing.
Signed-off-by: flakey5 <[email protected]>
Closes #214
TODO: