cli: generate man-page

[RedYetiDev]

This PR adds a tool to generate the node.1 file. See nodejs/api-docs-tooling#125

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (7b5d660) to head (71fe8c7).
Report is 81 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55268      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         653      654       +1     
  Lines      187476   187552      +76     
  Branches    36083    36081       -2     
==========================================
+ Hits       165763   165826      +63     
- Misses      14946    14959      +13     
  Partials     6767     6767              

see 25 files with indirect coverage changes

[ovflowd]

Can you attach comparison of the result before and after?

[ovflowd]

There are a lot of string manipulation operations happening here, and a lot of regular expressions.

It'd be great if you extracted those regex into constants and explained them and added more inline docs regarding the string operations you're doing and why (+ attaching relevant docs to the manfile spec that are relevant for the changes here)

I even wonder if this should live here? I have the feeling this could live under the api-docs-tooling as this is certainly an API documentation but for the CLI aspect of Node.

And since a the api-docs-tooling already has a lot of builtin pieces for manipulating and parsing the Markdown and generating things, maybe it'd be a good addition there, but that's a nit.

[ovflowd]

What makes this PR hard to review are the unknowns and little documentation, which in the end creates a "trust me, bro" sentiment since (at least I, as a reviewer) was given very little to be able to review these changes. It lacks more context and references to what is relevant for me to verify that it is working as expected.

+ It'd be great to have unit testing of the individual domain logic pieces of your tooling (extract them from the main function, into separate smaller functions, which allow us to better understand their in-and-outs)

I'd also love some JSDoc here, that would also be very helpful :)

[RedYetiDev]

Thanks for the detailed review!

I hadn't even thought of it before, but I think you're right that api-docs-tooling is a nice home for this, because as you said: all the processors are already there. I'm gonna work on implementing this there, so all these string manipulations won't need to happen as much. If that's cleaner, I'll move all of this there.

What makes this PR hard to review are the unknowns and little documentation, which in the end creates a "trust me, bro" sentiment since (at least I, as a reviewer) was given very little to be able to review these changes. It lacks more context and references to what is relevant for me to verify that it is working as expected.

Yes, I should add documentation...

---

Moving to draft until:
(A) Compared with a api-docs-tooling implemention
(B) JSDoc / Docs added
(C) Reduce the unknown variables

[ovflowd]

Appreciate you!

[RedYetiDev]

Ditto back to you :-)

[RedYetiDev]

See nodejs/api-docs-tooling#125, which implements this using the already established parsers, allowing for more details (more than 1 sentence) for the mandoc. Leaving this as a draft in case that is a no-go.

[ovflowd]

LGTM! Awesome stuff ✨

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63242/

[RedYetiDev]

I'm concerned that contributors may forget to regenerate this file when they modify the CLI file, so I'll add a network test (network to download api-docs-tooling) to ensure it's properly generated.

[RedYetiDev]

@nodejs/build this causes lint-md to use internet access, but not fail if the user does not have it. Is that something that the build team is okay with? #55266 seemed to be okay with it, and it required internet access.

[RedYetiDev]

There haven't been any objections in a few days to my comment above, if a new CI is started, can this land anytime soon?

[aduh95]

Instead of reading a file to delete it right away, why not use stdout? Also using sync operations seems unwise, and re-reading the same man file in the loop looks like a mistake

Suggested change

	child_process.execFileSync(process.execPath, [
	npxPath,
	'--yes',
	'github:nodejs/api-docs-tooling',
	'-i', path,
	'-o', '.tmp.1',
	'-t', 'man-page',
	]);
	manPageContent = fs.readFileSync('.tmp.1', 'utf-8');
	isManPageModified = manPageContent !== fs.readFileSync(manFilePath, 'utf-8');
	fs.rmSync('.tmp.1');
	const cp = child_process.spawn(process.execPath, [
	npxPath,
	'--yes',
	'github:nodejs/api-docs-tooling',
	'-i', path,
	'-o', '-',
	'-t', 'man-page',
	], { stdio: ['inherit', 'pipe', 'inherit']});
	manPageContent = Buffer.concat(await cp.stdout.toArray());
	isManPageModified = manPageContent.compare(actualManPageContent);

[RedYetiDev]

Yes. If nodejs/api-docs-tooling#137 lands, this will be a lot simpler, as child_process won't be needed at all.

[aduh95]

Using npx sounds like very bad DX, we will end up with different versions on different machines, won't we? I'm -1 on that, let's do like we do for linting the version number: the linter may or may not get the info from the env, if not it doesn't check it, and the CI can still report problems, and the devs stay happy:

node/.github/workflows/linters.yml

Lines 107 to 116 in 8de2695

	- name: Get release version numbers
	if: ${{ github.event.pull_request && github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch }}
	id: get-released-versions
	run: ./tools/lint-md/list-released-versions-from-changelogs.mjs >> $GITHUB_OUTPUT
	- name: Lint markdown files
	run: |
	echo "::add-matcher::.github/workflows/remark-lint-problem-matcher.json"
	NODE=$(command -v node) make lint-md
	env:
	NODE_RELEASED_VERSIONS: ${{ steps.get-released-versions.outputs.NODE_RELEASED_VERSIONS }}