-
Notifications
You must be signed in to change notification settings - Fork 23
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
remove api imports from examples #347
Conversation
@@ -13,10 +13,6 @@ | |||
#include <grpcpp/grpcpp.h> | |||
#include <grpcpp/support/status.h> | |||
|
|||
#include <viam/api/common/v1/common.pb.h> |
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.
⬆️ - Are the grpcpp
headers still required in this file and others?
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.
In this particular file no! But while we're on the subject gizmo/api.hpp
and summation/api.hpp
both include grpcpp/channel.h
. This could be replaced with the grpc_fwd.hpp
file but I'm not sure if it's worth the effort, thoughts?
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.
Or rather, not that it takes much effort but that it's a little awkward and perhaps it's a non-goal for users to not have google imports in headers
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.
I wouldn't bother in the ones that are doing custom APIs, since in that case I think the user is up to something complex, but for cases where it is just using the standard C++ SDK API I'd drop out the grpc headers if possible I think. Otherwise they will get copied in endlessly.
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.
agreed, I don't think we should scrub files with custom APIs.
Cherry-picked from #325