-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor: event id from const char* to string_view #469
base: master
Are you sure you want to change the base?
Conversation
void continuous_action_response::set_event_id(const char* event_id) { | ||
_event_id = event_id; | ||
void continuous_action_response::set_event_id(string_view event_id) { | ||
_event_id = std::string(event_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.
_event_id = std::string(event_id); | |
_event_id = std::string{event_id}; |
void continuous_action_response::set_model_id(const char* model_id) { | ||
_model_id = model_id; | ||
void continuous_action_response::set_model_id(string_view model_id) { | ||
_model_id = std::string(model_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.
_model_id = std::string(model_id); | |
_model_id = std::string{model_id}; |
void episode_history::update(const char* event_id, const char* previous_event_id, string_view context, const ranking_response& resp) { | ||
_depths[event_id] = this->get_depth(previous_event_id) + 1; | ||
void episode_history::update(string_view event_id, string_view previous_event_id, string_view context, const ranking_response& resp) { | ||
_depths[std::string(event_id)] = this->get_depth(previous_event_id) + 1; |
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.
This is not great... We get heterogeneous lookup in c++14 for map I believe. Doesn't help us here though
} | ||
|
||
std::string episode_history::get_context(const char* previous_event_id, string_view context) const { | ||
std::string episode_history::get_context(string_view previous_event_id, string_view context) const { | ||
return R"({"episode":{"depth":")" + std::to_string(this->get_depth(previous_event_id) + 1) + "\"}," + std::string(context.data() + 1); |
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.
Not part of this PR, but this should probably at least use a string stream
No description provided.