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

Closed
wants to merge 7 commits into from
Closed

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 (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
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 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
@RedYetiDev RedYetiDev force-pushed the mandoc-gen-test-update branch 2 times, most recently from 15c207e to 551d4f7 Compare October 23, 2024 21:13
@RedYetiDev
Copy link
Member Author

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

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?

@RedYetiDev RedYetiDev added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 1, 2024
@RedYetiDev RedYetiDev removed the blocked PRs that are blocked by other issues or PRs. label Nov 16, 2024
@RedYetiDev RedYetiDev marked this pull request as draft November 16, 2024 17:52
aduh95 and others added 3 commits November 16, 2024 12:57
PR-URL: nodejs#55855
Refs: nodejs#55333
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs#55856
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Raz Luvaton <rluvaton@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@RedYetiDev RedYetiDev marked this pull request as ready for review November 16, 2024 17:58
@RedYetiDev
Copy link
Member Author

Ack! The commits got mixed up.

@RedYetiDev
Copy link
Member Author

I'm going to recreate this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants