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

cli: generate man-page #55268

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Oct 4, 2024

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

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Oct 4, 2024
@RedYetiDev RedYetiDev added doc Issues and PRs related to the documentations. cli Issues and PRs related to the Node.js command line interface. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Oct 4, 2024
@RedYetiDev RedYetiDev marked this pull request as draft October 4, 2024 21:39
@RedYetiDev RedYetiDev marked this pull request as ready for review October 4, 2024 21:41
@RedYetiDev RedYetiDev marked this pull request as draft October 4, 2024 21:43
@RedYetiDev RedYetiDev force-pushed the mandoc-gen-test-update branch 2 times, most recently from f602715 to 9efb731 Compare October 4, 2024 21:55
@RedYetiDev RedYetiDev marked this pull request as ready for review October 4, 2024 21:56
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (7b5d660) to head (3b55d91).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55268      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files         653      653              
  Lines      187476   187476              
  Branches    36083    36090       +7     
==========================================
- Hits       165763   165761       -2     
- Misses      14946    14953       +7     
+ Partials     6767     6762       -5     

see 16 files with indirect coverage changes

@ovflowd
Copy link
Member

ovflowd commented Oct 14, 2024

Can you attach comparison of the result before and after?

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.

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

ovflowd commented Oct 14, 2024

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
Copy link
Member Author

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

@RedYetiDev RedYetiDev marked this pull request as draft October 14, 2024 11:42
@ovflowd
Copy link
Member

ovflowd commented Oct 14, 2024

Appreciate you!

@RedYetiDev
Copy link
Member Author

Ditto back to you :-)

@RedYetiDev
Copy link
Member Author

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.

@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. blocked PRs that are blocked by other issues or PRs. labels Oct 15, 2024
@RedYetiDev RedYetiDev changed the title test,cli: improve cli documentation tests with mandoc generation test,cli: generate man-page and use it for testing CLI.md Oct 16, 2024
@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Oct 16, 2024
@RedYetiDev RedYetiDev changed the title test,cli: generate man-page and use it for testing CLI.md cli: generate man-page Oct 21, 2024
@RedYetiDev RedYetiDev removed the blocked PRs that are blocked by other issues or PRs. label Oct 21, 2024
@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. and removed doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Oct 21, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review October 21, 2024 20:54
Copy link
Member Author

Choose a reason for hiding this comment

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

Review note: While this removes the testing for the man page content, I have a followup PR which will revamp this test file to include it.

I chose not to include it in this PR as it changes this file drastically, and requires changes to the CLI.md file, unrelated to this change.

@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Oct 21, 2024
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.

LGTM! Awesome stuff ✨

@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2024
@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 21, 2024
@RedYetiDev
Copy link
Member Author

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 RedYetiDev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 22, 2024
@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. cli Issues and PRs related to the Node.js command line interface. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants