Skip to content

chore: Fixes exit codes #141

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

chore: Fixes exit codes #141

wants to merge 1 commit into from

Conversation

sr010
Copy link
Contributor

@sr010 sr010 commented Aug 12, 2021

Fixes

Closes DII-45
Screenshot:
image

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@sr010 sr010 changed the base branch from main to release-feature-branch August 12, 2021 10:02
Sindhura3
Sindhura3 reviewed Aug 12, 2021
View reviewed changes
Copy link
Contributor

@Sindhura3 Sindhura3 left a comment

Choose a reason for hiding this comment

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

I think this fix can go into the main branch directly instead of the feature branch since there is no dependency on profiles or config.

src/base-commands/base-command.js
@@ -68,7 +68,7 @@ class BaseCommand extends Command {
this.logger.error(error.message);
this.logger.debug(error.stack);
}
this.exit(error.exitCode || 1);
this.exit(error.exitCode.toString().substring(0, 2) || 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we are already populating the API Error code in exit code, just not the expected format. Can we make the change in formatErrorMessage in cli-http-client instead of here. The error message can contain the full API error code but we'll trim and format for populating the exitCode in TwilioCliError there. And we may need a unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have addressed them here: #142

@sr010
Copy link
Contributor Author

sr010 commented Aug 12, 2021

I think this fix can go into the main branch directly instead of the feature branch since there is no dependency on profiles or config.

Will be raising a new pull request then

@sr010 sr010 closed this Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sindhura3 Sindhura3 Sindhura3 left review comments

@kridai kridai Awaiting requested review from kridai

@ravali-rimmalapudi ravali-rimmalapudi Awaiting requested review from ravali-rimmalapudi

@onuzbee onuzbee Awaiting requested review from onuzbee

@shrutiburman shrutiburman Awaiting requested review from shrutiburman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sr010 @Sindhura3