-
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
RSDK-9299 - refactor get_resource_name to be a non-proto method #329
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
#include <unordered_map> | ||
#include <vector> | ||
|
||
#include <boost/algorithm/string.hpp> | ||
#include <boost/blank.hpp> | ||
#include <boost/log/core.hpp> | ||
#include <boost/log/expressions.hpp> | ||
|
@@ -183,6 +184,26 @@ bool from_dm_from_extra(const ProtoStruct& extra) { | |
} | ||
return false; | ||
} | ||
std::pair<std::string, std::string> long_name_to_remote_and_short(const std::string& long_name) { | ||
std::vector<std::string> name_parts; | ||
// boost::split causes a clang-tidy false positive, see | ||
// https://bugs.llvm.org/show_bug.cgi?id=41141 | ||
// | ||
// NOLINTNEXTLINE | ||
boost::split(name_parts, long_name, boost::is_any_of(":")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it ever possible that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is possible. A long name is of form |
||
auto name = name_parts.back(); | ||
name_parts.pop_back(); | ||
auto remote_name = name_parts.empty() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should be checking Also could be good to add a unit test or two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait just saw your reply to Julie above, so split will return a vector with one element if there's no separator? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep! I've just added a test to highlight a couple cases just to be be explicit. |
||
? "" | ||
: std::accumulate(std::next(name_parts.begin()), | ||
name_parts.end(), | ||
*name_parts.begin(), | ||
[](const std::string& a, const std::string& b) { | ||
return a + ":" + b; | ||
}); | ||
|
||
return {std::move(remote_name), std::move(name)}; | ||
} | ||
|
||
} // namespace sdk | ||
} // namespace viam |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,10 @@ class ClientContext { | |
grpc::ClientContext wrapped_context_; | ||
}; | ||
|
||
/// @brief Given a fully qualified resource name, returns remote name (or "" if no remote name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is a remote name and short name? and what purpose do they server as opposed to resource names? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a remote name is the name of a remote that the resource is part of, and the short name is just the name of the resource. so we could have |
||
/// exists) and short name | ||
std::pair<std::string, std::string> long_name_to_remote_and_short(const std::string& long_name); | ||
|
||
/// @brief Returns a new `ProtoStruct` with a random key for server-side debug logging | ||
ProtoStruct debug_map(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,26 +2,17 @@ | |
|
||
#include <string> | ||
|
||
#include <google/protobuf/struct.pb.h> | ||
|
||
#include <viam/api/common/v1/common.pb.h> | ||
|
||
#include <viam/sdk/common/utils.hpp> | ||
#include <viam/sdk/resource/resource.hpp> | ||
|
||
namespace viam { | ||
namespace sdk { | ||
|
||
using viam::common::v1::ResourceName; | ||
|
||
Component::Component() : Resource("component"){}; | ||
Component::Component() : Resource("component") {} | ||
|
||
Component::Component(std::string name) : Resource(std::move(name)){}; | ||
Component::Component(std::string name) : Resource(std::move(name)) {} | ||
|
||
ResourceName Component::get_resource_name(std::string name) const { | ||
auto r = this->Resource::get_resource_name(name); | ||
*r.mutable_type() = kComponent; | ||
return r; | ||
Name Component::get_resource_name() const { | ||
return Resource::get_resource_name(kComponent); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does this function know what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
} // namespace sdk | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,21 +2,16 @@ | |
|
||
#include <string> | ||
|
||
#include <viam/api/common/v1/common.pb.h> | ||
|
||
#include <viam/sdk/common/utils.hpp> | ||
#include <viam/sdk/resource/resource.hpp> | ||
|
||
namespace viam { | ||
namespace sdk { | ||
|
||
common::v1::ResourceName Service::get_resource_name(std::string name) const { | ||
auto r = this->Resource::get_resource_name(name); | ||
*r.mutable_type() = kService; | ||
return r; | ||
Name Service::get_resource_name() const { | ||
return Resource::get_resource_name(kService); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as kComponent but where is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||
} | ||
|
||
Service::Service() : Resource("service"){}; | ||
Service::Service() : Resource("service") {} | ||
|
||
} // namespace sdk | ||
} // namespace viam |
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.
q: Why do we explicitly use fully qualified names like
boost::split
orstd::something
instead of bringing them into scope withusing
orusing namespace
in the code?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.
good question! I tend to like fully qualified names when they're not overly verbose. I appreciate the explicitness. And, a broader
using
line can add more than we need to scope and create potential for namespace collision.Also in this case,
boost::split
andboost::is_any_of
are only used once each in this file sousing
them would actually make the code (slightly) more verbose, not less.