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 error code name to status error messages #126

Merged

Conversation

murgatroid99
Copy link
Member

We frequently get questions that reference status errors, but they almost never realize that the code is important or know what it means. This adds the status code number and name to the error message.

let message = `${status.code} ${status.name}: ${status.details}`;
let error = new Error(message);
error.code = status.code;
error.details = status.details;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include error.metadata as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That is actually what I meant to write there. The details string is in the message. It probably doesn't need to be in a separate field too.

@kjin
Copy link
Contributor

kjin commented Dec 15, 2017

LGTM once tests are amended to reflect new message changes

@murgatroid99
Copy link
Member Author

I was lazy and didn't run the tests, so I didn't realized that I had so many tests that depended on the specific text of the error messages. I'll have to think about how to handle that.

@murgatroid99
Copy link
Member Author

My solution is to add a details string to the error objects that exactly matches the original details string, and check that in the tests.

@murgatroid99 murgatroid99 merged commit 28fcd46 into grpc:v1.8.x Jan 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants