Skip to content
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

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Oct 14, 2024

Description

This PR adds support for the generation of the node.1 Manfile automatically based on the cli.md input.

Validation

$ node bin/cli.mjs -t man-page -i doc/api/cli.md -o doc/node.1

The above command should provide a valid manfile (runnable with man ./doc/node.1).

Related Issues

nodejs/node#55268

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I've covered new added functionality with unit tests if necessary.

src/generators/index.mjs Outdated Show resolved Hide resolved
@RedYetiDev
Copy link
Member Author

I've made (most of) the requested changes. Thanks for the reviews!

Copy link
Member

@flakey5 flakey5 left a 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

src/generators/man-page/utils/converter.mjs Show resolved Hide resolved
src/generators/man-page/utils/converter.mjs Show resolved Hide resolved
src/generators/man-page/utils/converter.mjs Show resolved Hide resolved
Copy link
Collaborator

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !!!

@RedYetiDev RedYetiDev changed the title feat: add mandoc generator feat: add man-page generator Oct 16, 2024
@RedYetiDev
Copy link
Member Author

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 mman -Thtml).

Copy link
Member

@ovflowd ovflowd left a 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 AugustinMauroy merged commit 4dbfd05 into nodejs:main Oct 21, 2024
6 checks passed
@ovflowd
Copy link
Member

ovflowd commented Oct 21, 2024

@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'
Copy link
Member

@ovflowd ovflowd Oct 21, 2024

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');
Copy link
Member

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

@ovflowd
Copy link
Member

ovflowd commented Oct 21, 2024

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.

@ovflowd
Copy link
Member

ovflowd commented Oct 21, 2024

@RedYetiDev I finished reviewing the latest change :)

Would you mind making a follow-up PR 🙈?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants