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

Add override to allow throw instead of process.exit #1040

Merged
merged 9 commits into from
Sep 20, 2019

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Sep 9, 2019

Pull Request

Problem

Commander has multiple calls to process.exit which is hard to override to use as a library and parse multiple strings, as parsing errors or --help terminate the process. Doing a throw instead would allow appropriate handling and carry on.

Issues: #945 #575 #934

Solution

Add .exitOverride(callback) routine to do custom handling of code that would otherwise call process.exit(). The callback is optional and the default behaviour is to throw an instance of CommanderError which includes the exitCode, a string code, and a message.

This allows writing code like:

const program = new commander.Command();
program
  .exitOverride()
  .option(-p, --pepper <type>, 'add pepper');

try {
  program.parse(['node', 'test', '--pepper']);
} catch (err) {
  // custom processing
}

(The routine name exitOverride is a bit implementation specific rather than friendly, but this is an optional feature and not sure how much or how it will be used.)

Limitations

  • code flow is not changed, so override still expected to throw or exit and not fall through
  • executable subcommand calls spawn as async, not currently overriding those process.exit as different pattern required
  • writing to stdout and stderr as normal before override

ToDo

  • TypeScript
  • README
  • not doing tests here as have extensive tests in feature/jest branch

Making the code visible for discussion and review.

@shadowspawn
Copy link
Collaborator Author

One of the prompts for adding this feature was to simplify writing unit tests. The test file for exitOverride itself (on a different branch) is:

https://github.com/shadowspawn/commander.js/blob/feature/jest/tests-jest/command.exitOverride.test.js

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

LGTM!

@shadowspawn
Copy link
Collaborator Author

Thanks @abetomo 😄

@shadowspawn shadowspawn added this to the v4.0.0 milestone Sep 14, 2019
@shadowspawn
Copy link
Collaborator Author

Added support for async calls in executableSubcommand.

@shadowspawn
Copy link
Collaborator Author

Allows custom exit codes: #188 #436

@shadowspawn
Copy link
Collaborator Author

The implementation already landed in the Jest PR. This PR now effectively promotes it from private _exitOverride to public exitOverride.

Added description to README.

Suggested line for CHANGELOG:

@shadowspawn shadowspawn changed the title WIP: Add override to allow throw instead of process.exit Add override to allow throw instead of process.exit Sep 20, 2019
@shadowspawn shadowspawn merged commit 8df692e into tj:develop Sep 20, 2019
@shadowspawn shadowspawn deleted the feature/exitOverride branch September 20, 2019 23:07
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Sep 20, 2019
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Nov 2, 2019
@lxsmnsyc
Copy link

Hello, I was looking for a solution to prevent "--help" from exiting the CLI.

I tried using exitOverride, but it doesn't really prevent it from exiting. Is there any way for this?

@shadowspawn
Copy link
Collaborator Author

@lxsmnsyc
exitOverride would be my suggestion. Open a new issue with some more details. (I am not sure what you mean by "doesn't really prevent it from exiting".)

@lxsmnsyc
Copy link

lxsmnsyc commented Nov 29, 2019

@lxsmnsyc
exitOverride would be my suggestion. Open a new issue with some more details. (I am not sure what you mean by "doesn't really prevent it from exiting".)

I have found a work around to my problem.

I have to take a look at commander's index js to know how it executes commands, and to understand the behavior in general :)

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.

3 participants