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: move json parser into its own lib #4381
refactor: move json parser into its own lib #4381
Conversation
@@ -2,324 +2,6 @@ | |||
|
|||
#include "vw/common/future_compat.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.
explicit template initialization needs to stick around here and in parse_example_json.cc
until the deprecated functions are removed
template <typename... ArgsT> | ||
std::unique_ptr<VW::config::options_i> make_args(ArgsT const&... args) | ||
{ | ||
return std::unique_ptr<VW::config::options_cli>(new VW::config::options_cli(std::vector<std::string>({args...}))); | ||
} | ||
|
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.
parse_json/dsjson still used in most unit_tests so need to be available
} | ||
|
||
template <bool audit> | ||
void read_line_json_s(const VW::label_parser& lbl_parser, hash_func_t hash_func, uint64_t hash_seed, |
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.
What does _s
mean?
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 am guessing string, we could rename it to plain read_line_json
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.
Maybe after this merged we can do a pass cleaning up the functions a bit? We can leave it to later to make this one easier though
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.
yes we should, I also wanted to make some char*/size -> string_view transistions but the PR is big already
…wabbit into move_json_parser_to_lib
vowpalwabbit/json_parser/include/vw/json_parser/decision_service_utils.h
Outdated
Show resolved
Hide resolved
…wabbit into move_json_parser_to_lib
and move json tests to gtests