-
Notifications
You must be signed in to change notification settings - Fork 5
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
Split gRPC and HTTP error utility into seperate packages #5
Conversation
pkg/grpc/grpc.go
Outdated
@@ -14,7 +14,7 @@ | |||
limitations under the License. | |||
*/ | |||
|
|||
package errdefs | |||
package grpc |
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.
errdefs.FromGRPC should be now called like grpc.ToNative ?
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.
Yeah, grpc.FromGRPC
can be redundant. grpc.ToNative
is nice naming imo. Pushed to try it out.
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.
Nit: maybe grpcerror
or something?
I just dislike having package name conflicts that makes goimports trip up.
69d2263
to
cee4adc
Compare
cee4adc
to
702b90e
Compare
Thanks for doing the split
+1 to keeping it at the top. I see
+1 for keeping err (or error) in the package name (not the directory name). Maybe to be like...
? @cpuguy83 that what you were thinking? |
Not really, but that's fine. |
334de29
to
8b9db8f
Compare
By having seperate packages, users can consume base package without pulling gRPC or HTTP as a dependency if not required. Signed-off-by: Austin Vazquez <macedonv@amazon.com>
8b9db8f
to
fd0e482
Compare
Ok, works for me |
Issue:
#4
Description:
By having a seperate packages, users can consume base package without pulling gRPC or HTTP as a dependency if not required.
This approach creates
errgrpc
anderrhttp
. The imports for users would be the following:Other approaches considered:
Top level
grpc
andhttp
packages. The imports for users would be the following:My thought was this may clutter the top level directory, but this may be acceptable as the number of protocol packages would be finite.
Testing:
Adds basic unit tests for there and back again error conversions for gRPC and HTTP error packages.