Skip to content

Conversation

@greghuels
Copy link
Collaborator

@greghuels greghuels commented Dec 2, 2025

Summary

Introduces a new parseConfiguration() helper function that simplifies configuration setup for SDK users.

Example:

  auto result = eppoclient::parseConfiguration(flagConfigJson, banditModelsJson);
  if (!result.hasValue()) {
      std::cerr << "Configuration parsing failed:" << std::endl;
      for (const auto& error : result.errors) {
          std::cerr << "  - " << error << std::endl;
      }
      return 1;
  }
  configStore.setConfiguration(std::move(*result.value));

@greghuels greghuels force-pushed the greg.huels/parse-configuration-helper branch 3 times, most recently from a0917ea to 27cc3a6 Compare December 2, 2025 14:40
@greghuels greghuels requested a review from dd-oleksii December 2, 2025 14:40
@greghuels greghuels mentioned this pull request Dec 2, 2025
@greghuels greghuels force-pushed the greg.huels/FFESUPPORT-348/fix-end-date-parsing-2 branch from cbdffd1 to 62a765b Compare December 2, 2025 14:47
@greghuels greghuels force-pushed the greg.huels/parse-configuration-helper branch 2 times, most recently from 5997d33 to bbd9550 Compare December 2, 2025 19:24
@greghuels greghuels force-pushed the greg.huels/parse-configuration-helper branch from bbd9550 to 739d310 Compare December 2, 2025 21:07
@greghuels greghuels changed the base branch from greg.huels/FFESUPPORT-348/fix-end-date-parsing-2 to main December 2, 2025 21:07
@greghuels greghuels force-pushed the greg.huels/parse-configuration-helper branch from 739d310 to a08c2ec Compare December 2, 2025 21:11
}

ParseResult<Configuration> parseConfiguration(const std::string& flagConfigJson,
const std::string& banditModelsJson) {
Copy link
Collaborator

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.)

Copy link
Collaborator Author

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

Comment on lines 65 to 66
* @param banditModelsJson JSON string containing bandit models (optional - pass empty string to
* skip)
Copy link
Collaborator

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

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@greghuels greghuels merged commit c9c608b into main Dec 3, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants