-
Notifications
You must be signed in to change notification settings - Fork 8
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
Don't return rpc.Status as an explicit response data item #148
Comments
avive
changed the title
Don't return grpc status as repsonse members
Don't return rpc.Status as an explicit response data items
Jun 1, 2021
avive
changed the title
Don't return rpc.Status as an explicit response data items
Don't return rpc.Status as an explicit response data item
Jun 1, 2021
lrettig
added
good first issue
Good for newcomers
help wanted
Extra attention is needed
labels
Jun 12, 2021
I think you're probably right and I think we should probably do this 👍 |
I've implemented this in the 0.3 branch |
avive
removed
good first issue
Good for newcomers
help wanted
Extra attention is needed
labels
Jun 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
All rpc api method calls started by clients return a status code and optional message.
I think that adding a status explicitly in responses is redundant and def not idiomatic grpc pattern.
e.g.:
There are several other responses such as this in the smesher service.
So a method that just returns a status in its response should use the build-in status and status code.
See: https://grpc.io/docs/guides/error/
So a method that completes without an error and has not data to return such as StopSmeshingResponse should return status == 0 when there's no error or otherwise return one of the pre-defined error statuses and a message as defined here: https://grpc.github.io/grpc/core/md_doc_statuscodes.html - or an un-reserved code with message when one of the reserved error codes is not applicable.
@lrettig - please review.
So, StopSmeshingResponse should be defined like this:
The following status codes are never generated by the library and can be used:
The text was updated successfully, but these errors were encountered: