RSDK-6627 - move resource-names-for-resource to be a method on resource #222
Conversation
stuqdog
requested review from
njooma and
purplenicole730
and removed request for
a team
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
If you want to keep this to pure code motion to limit scope then LGTM. If you are willing to put a few more minutes into it, I think it'd be nice to see a doc comment, and maybe bikeshed the name of the function? I'm a little unclear on what it is intended to do.
src/viam/sdk/resource/resource.hpp
Outdated
| /// @brief Return the resource's name. | ||
| virtual std::string name() const; | ||
|
|
||
| std::vector<Name> resource_names() const; |
There was a problem hiding this comment.
Let's please add a doc comment for this while we are moving it.
Also, is resource_names the best name for this method? I'm actually a little unclear on what the meaning of the returned names is. It seems to be all the registered models that share the same API as this resource?
There was a problem hiding this comment.
Done! Though, it appears the only place this method is ever used is on a private method in RobotService. I wonder if it might make sense to just have this be a private method and make RobotService a friend class?
src/viam/sdk/robot/service.cpp
Outdated
| for (const auto& key_and_val : resource_manager()->resources()) { | ||
| for (const Name& resource : resource_names_for_resource(key_and_val.second)) { | ||
| metadata.push_back(resource.to_proto()); | ||
| for (const Name& name : key_and_val.second->registered_models()) { |
There was a problem hiding this comment.
If this is really the only usage of registered_models, I'd suggest just moving the function into this file in the unnamed namespace and changing it to take a const Resource&
3 participants
No description provided.