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

feat: allow non-cli execution #137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

RedYetiDev
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.

package.json Outdated Show resolved Hide resolved
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 !

@RedYetiDev
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.

src/index.mjs Outdated Show resolved Hide resolved
src/index.mjs Outdated Show resolved Hide resolved
@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?

@RedYetiDev
Copy link
Member Author

RedYetiDev 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!

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