-
Notifications
You must be signed in to change notification settings - Fork 214
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
Replace process.exit()
with throwing an error
#233
base: master
Are you sure you want to change the base?
Conversation
Using process.exit() does not allow us to use this tool programmatically, as we cannot then handle the error in a custom way (ie: formatting the response for CI, logging it, somewhere, etc). This PR replaces logging to `console.error` + using `process.exit` with throwing an error. This will have the same effect in CLI usage by immediately terminating the process with a message, but can also be `try/catch` handled in a parent program.
process.exit()
with throwing an errorprocess.exit()
with throwing an error
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.
The tests need to be adjusted for this!
Can you give me a hint where to look? The test seem to be OK on this PR... |
I imagine those tests are still passing because a |
I guess you are right, at least for the "bin/license-checker" tests. |
I'm sorry, but I still don't think I follow what you are asking for. I'm looking at this example: describe('should exit on given list of onlyAllow licenses', function() {
var result={};
before(parseAndFailOn('onlyAllow', '../', "MIT; ISC", result));
it('should exit on non MIT and ISC licensed modules from results', function() {
assert.equal(result.exitCode, 1);
});
}); from (https://github.com/davglass/license-checker/blob/master/tests/test.js#L233-L240) The test runs |
OH! the test suite is not passing with these changes, even though travis reports success. I see the problem now :D |
The function https://github.com/davglass/license-checker/blob/master/tests/test.js#L215-L231 |
Using
process.exit()
does not allow us to use this tool programmatically, as we cannot then handle the error in a custom way (ie: formatting the response for CI, logging it somewhere, etc).This PR replaces
console.error
+ usingprocess.exit
with throwing an error. This will have the same effect in CLI usage by immediately terminating the process with a message, but can also betry/catch
handled in a parent program.A downside might be that the error output is less terse... but it will be more noticeable!