-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: improve errors by hoisting ErrorInfo fields directly in error message #354
Conversation
Requesting @noahdietz , @bshaffer as PHP owners, and @summer-ji-eng as Error Improvement project lead for review. This is a non urgent review as we will not be able to release this until post freeze. Thanks! |
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.
A few comments to start. Can you also run the php formatting tool? Should be in the readme somewhere.
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.
Can you add php doc comments to all of the added public APIs?
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.
Does this implementation cover for REST mode?
Yup! I added a test for the error info in REST response. Good catch. |
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.
LGTM! Great stuff Alice. I will let @bshaffer review/Approve.
Thanks Alice for your work. LGTM. |
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.
Some suggestions for cleanup, otherwise looks great
758f6e6
to
d52e17f
Compare
As part of the overall GAPIC Error Details Improvements project, this feature improves the usability of the error information by including three additional fields (if available) directly in the error itself e.g. ErrorInfo that offer more detailed information for troubleshooting. These three fields are
domain
,reason
, andmetadata
.This PR also includes getters for those fields so users may access them directly.
This PR is dependent on googleapis/common-protos-php#27 being merged which will not happen until post-freeze, sometime in January.
Please ping me for internal docs if desired.
Current REST sample error:
With this change, REST sample error:
Current GRPC sample error:
With this change, GRPC sample error: