Update SDK to allow evaluation for more than just boolean flags.#64
Conversation
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Summary of ChangesHello @NeaguGeorgiana23, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenFeature C++ SDK by extending its capability to evaluate feature flags beyond simple booleans. It introduces support for string, integer, double, and object flag types, making the SDK more versatile for various application requirements. The core change involves a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, extending the SDK to support evaluation for string, int, double, and object flags. The refactoring in ClientAPI to use a templated EvaluateFlag method is a good improvement to reduce code duplication, and the new Value class provides a flexible way to handle different data types. I've identified a few issues, including some critical bugs in the new Value class that could lead to compilation errors or runtime crashes. I've also noted some areas for improvement regarding dependencies and use of the override specifier. My detailed comments and suggestions are below.
| std::unique_ptr<StringResolutionDetails> ClientAPI::EvaluateStringFlag( | ||
| std::string_view flag_key, std::string_view default_value, | ||
| const std::optional<EvaluationContext>& ctx) { | ||
| std::string default_str(default_value); |
There was a problem hiding this comment.
To avoid creating a temporary std::string from the std::string_view default_value, you can pass default_value directly to EvaluateFlag and construct the std::string inside the lambda. This is slightly more efficient as the string is only constructed if the provider call is actually made.
return this->EvaluateFlag<StringResolutionDetails>(
default_value, ctx,
[&](const std::shared_ptr<FeatureProvider>& provider,
const EvaluationContext& merged_ctx) {
return provider->GetStringEvaluation(flag_key, default_value,
merged_ctx);
});
There was a problem hiding this comment.
If I implement this change, the generic EvaluateFlag will try to create a ResolutionDetailsType with a std::string_view instead of a std::string as a default_value. This results in a type mismatch error. To make this work, inside EvaluateFlag, I need to check if ResolutionDetailsType is StringResolutionDetails and convert std::string_view to std::string. Do you think this is worth it for the optimization above? If yes, lmk and I'll do the necessary changes.
There was a problem hiding this comment.
Okay, I see the templated logic - you can leave it as is
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
…into extend_sdk
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
| std::unique_ptr<StringResolutionDetails> ClientAPI::EvaluateStringFlag( | ||
| std::string_view flag_key, std::string_view default_value, | ||
| const std::optional<EvaluationContext>& ctx) { | ||
| std::string default_str(default_value); |
There was a problem hiding this comment.
Okay, I see the templated logic - you can leave it as is
3 participants
This PR
Adds extends the SDK such that it will be able to evaluate flags of type string, int, double and object.
Related Issues
Fixes #63