-
Notifications
You must be signed in to change notification settings - Fork 19
feat: add man-page generator
#125
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
Conversation
|
I've made (most of) the requested changes. Thanks for the reviews! |
flakey5
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.
lgtm at least, left a few nits regarding spacing to align it with the style of the other files
AugustinMauroy
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.
LGTM !!!
|
If your curious what this looks like, an HTML-compiled version is available at https://redyeti.dev/hostables/nodejs/1729108143-new-man-page.html. (Converted using |
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. It is important to mention that code style, and DevEx are higher priorities in this repository than performance. This is not the Node runtime; it is just tooling to generate docs.
I'd say that the focus here should be maintainability and not performance.
That said, excellent work so far!
|
@AugustinMauroy, why did you merge this? It got approved, but I was waiting for @redyetidev to push so I could give a final look 😅 |
| const environmentStart = input.findIndex( | ||
| const optionsStart = components.findIndex(({ slug }) => slug === 'options'); | ||
| const environmentStart = components.findIndex( | ||
| ({ slug }) => slug === 'environment-variables-1' |
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 probably be a constant (including the options one above) and reference where from the CLI.md source we are looking for this.
If that file ever gets updated with this content removed, we're screwed.
| // Filter to only 'cli'. | ||
| const components = input.filter(({ api }) => api === 'cli'); | ||
| if (!components.length) { | ||
| throw new Error('CLI.md not found'); |
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 could have been a more laid-out error message
|
Also, @AugustinMauroy, this repository is outside of the Node.js Website team scope. It is more in the scope of the Web Infra team + Documentation team, so I'd avoid merging PRs -- but also my bad here. I gave approval, but my comment was a bit explicit that this was not ready yet. |
|
@redyetidev I finished reviewing the latest change :) Would you mind making a follow-up PR 🙈? |
Description
This PR adds support for the generation of the
node.1Manfile automatically based on thecli.mdinput.Validation
The above command should provide a valid manfile (runnable with
man ./doc/node.1).Related Issues
nodejs/node#55268
Check List