-
Notifications
You must be signed in to change notification settings - Fork 22
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
RSDK6734 - Remove viam/api imports from public headers #325
base: main
Are you sure you want to change the base?
RSDK6734 - Remove viam/api imports from public headers #325
Conversation
…qdog/cpp-sdk into make-proto-conversions-private
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.
Note the existence of this is only to hide the google/protobuf
import from the public client_helper.hpp
ResourceConfig(std::string type, | ||
std::string name, | ||
std::string namespace_, | ||
ProtoStruct attributes, | ||
std::string api, | ||
Model model, | ||
LinkConfig frame); |
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.
Note the new constructor here. It's just taking the component parts.
const std::vector<geometries_in_frame>& obstacles() const; | ||
const std::vector<transform>& transforms() const; |
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.
note the new getters here
/// @brief Returns the `Name` for a particular resource. | ||
virtual Name get_resource_name() const; |
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.
Note that the get_resource_method
name here has changed to give us our own Name
type rather than the proto ResourceName
. We can use the conversion methods from that Name
when necessary.
translation get_translation() const; | ||
OrientationConfig get_orientation_config() const; | ||
GeometryConfig get_geometry_config() const; | ||
std::string get_parent() const; | ||
|
||
private: | ||
std::string id_; |
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.
Note the removal of this id_
field, which was unused and inaccessible.
So I think there's a lot here we'll want to use going forward but also some decisions which I think we might want to give some more thought before jumping into. My short term recommendation would be that probably the changes to Some further thoughts to elaborate:
// forward declare
namespace viam::api::v1 { class SomeComponent; }
void to_proto(const SomeComponent& source, viam::api::v1::SomeComponent* dest) We probably wan to distinguish our approaches between
I think the first case is more insulated and well understood but there's more open ended stuff for the second case. In the second case I think we might end up with something that looks like the forward declaration and pointer-based conversion function above appearing in |
Cool, thanks for the comprehensive reply! I think it might be good to talk non-async on this since there's probably going to be a fair amount of back and forth, but one quick high level clarification that I think will help drive that conversation:
So as I understand it, a major goal here is that from a user experience point of view, proto no longer exists. The proto conversions header is intentionally private private specifically because a user should never have to do proto conversions with any of our internal types. So I'm not sure we should be worrying about how a user finds this, or including any of this in |
@stuqdog we can definitely talk non-async! Also going to cc @acmorrow if he has time to weigh in, but regarding the "proto no longer exists" point, I think the goal is rather that proto should not comprise any part of our API nor ABI, so that
That said, in some cases it's possible that users will still want to access proto types directly as an escape hatch for some advanced use case not covered by the API surface we provide, and if so they can make proto a public dependency of their module, |
…o make-proto-conversions-private
Moves all(1) proto conversion out of public files and into either the relevant client or server (when specific to a particular resource), or into the new
private/proto_conversions.xpp
(when used in multiple places). The one exception isprivate/encoder.xpp
, which contains methods only used for encoders, but used by both client and server code.A lot of this can be skimmed over/ignored, as it's just mechanical moves of methods with straightforward changes (e.g., taking an object of type
T
as an arg instead of being a method in classT
).Notable changes are as follows (I have tried to add comments indicating where these occur as well):
private/client_helper.hpp
. all it does is#include <google/protobuf/struct.pb.h>
so that the public client helper doesn't have to.id_
field fromLinkConfig
type. This was never used and was impossible to access. Technically breaking since the shape of aLinkConfig
has changed.ResourceConfig
) and getters (seeWorldState
). Shouldn't be anything controversial, they getters returnconst&
and the constructors just take the data members.get_resource_name
in a resource to 1) no longer take aname
argument (we were always just using the resource name so we might as well not ask for it), and 2) return aName
instead of a protobufResourceName
. This is technically breaking though I doubt anyone was relying on this method to return aResourceName
before.Also a couple flybys here and there (mostly removing unused imports, removed some proto conversion methods that were unused, removing an unused
stub
created in thedial
method).(1) technically
viam/api
imports still exist forrobot_server.hpp
andmodule_service.hpp
. Given that these are both explicitly defining an implementation of a proto type, and that we want these to be public, I question whether removing theviam/api
imports from these is possible or desirable.