refactor: all exits moved to cli, throw as needed #157
Conversation
|
@RA80533 I would love your review here as well. :) |
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
==========================================
+ Coverage 91.57% 92.17% +0.60%
==========================================
Files 15 15
Lines 546 537 -9
Branches 105 101 -4
==========================================
- Hits 500 495 -5
+ Misses 20 16 -4
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@RA80533 this is now merged, so refactoring globs / io will likely be a lot easier. |
* chore(deps): update dependency eslint to v7.29.0 (#150) * chore(deps): update alpine docker tag to v3.14.0 (#144) * chore(deps): update dependency jest to v27.0.5 (#156) * fix: correct flags regex to match server and docs (#147) * refactor: all exits moved to cli, throw as needed (#157) * chore: change headers to standard (#158) Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Drazi Crendraven <drazisil@users.noreply.github.com>
| app.main(argv).then(() => { | ||
| const end = Date.now() | ||
| app.log(`End of uploader: ${end - start} milliseconds`, { level: 'debug', argv }) | ||
| }).catch(error => { | ||
| app.log(`Error!: ${error}`, { level: 'error', argv }) | ||
| const end = Date.now() | ||
| app.log(`End of uploader: ${end - start} milliseconds`, { level: 'debug', argv }) | ||
| process.exit(argv.nonZero ? -1 : 0) | ||
| }) |
There was a problem hiding this comment.
The idea here is perfect.
The finally() method can be used here in the same manner as the traditional try-catch-finally construct:
try {
somethingThatWillThrowAnError();
} catch (reason) {
handleTheError(reason);
} finally {
// Close out any file handles, etc.
performAnyCleanUpToTheEnvironment();
}app.main(argv).catch(error => { /* … */ }).finally(() => { /* … */ });| app.main(argv).then(() => { | |
| const end = Date.now() | |
| app.log(`End of uploader: ${end - start} milliseconds`, { level: 'debug', argv }) | |
| }).catch(error => { | |
| app.log(`Error!: ${error}`, { level: 'error', argv }) | |
| const end = Date.now() | |
| app.log(`End of uploader: ${end - start} milliseconds`, { level: 'debug', argv }) | |
| process.exit(argv.nonZero ? -1 : 0) | |
| }) | |
| app.main(argv).catch(error => { | |
| app.log(`Error!: ${error}`, { level: 'error', argv }) | |
| process.exitCode = argv.nonZero ? -1 : 0 | |
| }).finally(() => { | |
| const end = Date.now() | |
| app.log(`End of uploader: ${end - start} milliseconds`, { level: 'debug', argv }) | |
| }) | |
| }) |
There was a problem hiding this comment.
One thing to keep in mind is that setting the exitCode property of process will not cause the program to immediately terminate. Node.js will run its course throughout whatever execution flow it's currently running but the program will terminate with the assigned exit code. This is more often the appropriate pattern to use (instead of process.exit(), etc.).
The official documentation (here) has more information pertaining to how Node.js terminates.
|
I meant to push my review comments as I made them rather than bundle them together into a review. I'm not sure if these comments were visible prior to the merge. |
Nope, but that's good to know about the exitCode. Normally I would want to quit on the spot , but in this case since I duplicate code, your suggestions makes more sense. |
This makes the main() method testable.