-
Notifications
You must be signed in to change notification settings - Fork 1
feat: parseConfiugration helper #42
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
Conversation
a0917ea to
27cc3a6
Compare
cbdffd1 to
62a765b
Compare
5997d33 to
bbd9550
Compare
bbd9550 to
739d310
Compare
739d310 to
a08c2ec
Compare
| } | ||
|
|
||
| ParseResult<Configuration> parseConfiguration(const std::string& flagConfigJson, | ||
| const std::string& banditModelsJson) { |
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.
minor: we have a configuration wire format that combines both flags and bandits configuration in the same json. I wonder if we should reserve parseConfiguration for that.
(From public API perspective, I think we only want to expose this configuration wire parsing as this is our intended format for passing configuration to/from SDKs.)
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 think this is fine, since we're likely not going to need to pull in configuration wire functionality in this SDK
src/configuration.hpp
Outdated
| * @param banditModelsJson JSON string containing bandit models (optional - pass empty string to | ||
| * skip) |
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.
nit: we might provide another parseConfiguration override that only takes a single argument, so this "pass empty string" is not exposed to user
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 think that comment wasn't necessary to begin with, since it's an optional parameter. Though adding the override is probably a bit more clear, so I'll update.
| run-bandits: examples | ||
| @echo "Running bandits example..." | ||
| @$(BUILD_DIR)/bandits | ||
| @cd examples && ../$(BUILD_DIR)/bandits |
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.
nit: do we expect examples to run from ./examples directory? Not opposed to that but that seems inconsistent with tests, that run from repository root
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 think this makes more sense than updating the relative paths in the examples, yeah. These are meant to be self-contained examples that the user could copy if they wanted.
Summary
Introduces a new
parseConfiguration()helper function that simplifies configuration setup for SDK users.Example: