-
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: add man-page
generator
#125
Conversation
3ce0d19
to
6f5c34a
Compare
I've made (most of) the requested changes. Thanks for the reviews! |
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
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 !!!
4b83bc0
to
de3abec
Compare
de3abec
to
e535c13
Compare
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!
0d39386
to
889dec8
Compare
889dec8
to
5e1e0ce
Compare
@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 optionsStart = input.findIndex(({ slug }) => slug === 'options'); | ||
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.1
Manfile automatically based on thecli.md
input.Validation
The above command should provide a valid manfile (runnable with
man ./doc/node.1
).Related Issues
nodejs/node#55268
Check List