Skip to content

Conversation

@avivkeller
Copy link
Member

Description

This PR adds the ability to execute this from another JS file, without the use of the CLI. That way, man-page, and any future generators wouldn't be limited to the CLI for execution

Validation

Verify that all execution is working as planned.

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 November 2, 2024 16:48
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.

LGMT !

@avivkeller
Copy link
Member Author

Thanks for the approvals! Once this lands, I'll be able to implement it with nodejs/node#55268, which will really bring the new man-page generator into the core :-)!

@AugustinMauroy
Copy link
Member

cc @ovflowd

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.

Ill explain better why Im against this implementation later, although Im +1 with the idea of allowing this to be consumed within JS, I just want to prevent someone to merge this for the time being.

@ovflowd
Copy link
Member

ovflowd commented Nov 5, 2024

Alrighty, I'm back 👋

So pretty much my idea here is:

Instead of exporting an one does everything function, cli.js should still do exactly what it is doing, parsing cli args and doing whatever it needs to be done to be able to run from the CLI.

On index.js simply only export all the functions that exist on the other modules. My reasoning is that by exporting this nebulous function it creates a black box and prevents people from actually importing each module.

Hence on node core you'll simply import the exports of this package and create your own instance of whatever you need, which allows you even to make it specific for the needs of man doc gen.

Also don't forget to update package.json so that only the index.mjs file is exported on the module.

How do you feel about this?

@avivkeller avivkeller requested a review from ovflowd November 6, 2024 22:28
@avivkeller
Copy link
Member Author

avivkeller commented Nov 8, 2024

Thanks for your feedback. I've updated this to just export the other files. PTAL when you get a chance :-)

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.

Here we go :)

@ovflowd
Copy link
Member

ovflowd commented Nov 8, 2024

Thank you, @redyetidev and apologies for blocking you!

@avivkeller avivkeller merged commit 24aaa11 into nodejs:main Nov 16, 2024
6 checks passed
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