-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
cli: generate man-page #55268
Conversation
fc27a80
to
7a99181
Compare
7a99181
to
343508c
Compare
f602715
to
9efb731
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
Can you attach comparison of the result before and after? |
There was a problem hiding this 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.
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 :) |
Thanks for the detailed review! I hadn't even thought of it before, but I think you're right that
Yes, I should add documentation... Moving to draft until: |
Appreciate you! |
Ditto back to you :-) |
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. |
CLI.md
9efb731
to
1615b2d
Compare
CLI.md
15c207e
to
551d4f7
Compare
@nodejs/build this causes |
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? |
71fe8c7
to
48b37ac
Compare
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>
Ack! The commits got mixed up. |
I'm going to recreate this PR. |
This PR adds a tool to generate the
node.1
file. See nodejs/api-docs-tooling#125