Skip to content

Conversation

@avivkeller
Copy link
Member

@avivkeller avivkeller 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.

@avivkeller avivkeller requested a review from a team as a code owner October 14, 2024 16:43
@avivkeller
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

Copy link
Member

@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 !!!

@avivkeller avivkeller changed the title feat: add mandoc generator feat: add man-page generator Oct 16, 2024
@avivkeller
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 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