Client API#53
Conversation
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
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 advances the OpenFeature C++ SDK by introducing foundational client-side feature flag evaluation capabilities. It establishes the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces the ClientAPI as a concrete implementation of the Client interface, and adds the GlobalContextManager for handling global evaluation context. The changes include the implementation for boolean flag evaluation and corresponding build file updates and tests.
My review has identified a few issues:
- The context merging logic in
ClientAPIis incomplete and does not adhere to the OpenFeature specification as it ignores the global context. - The
GlobalContextManagerimplementation has a missing header include which could lead to build failures on some platforms. - A test file includes a
mainfunction which will conflict with thegtest_mainlibrary used in the build, causing a linker error.
Details and suggestions for these points are in the specific comments.
| EvaluationContext ClientAPI::MergeContexts( | ||
| const std::optional<EvaluationContext>& invocation_ctx) { | ||
| // TODO: Add context merging logic after EvaluationContext is implemented. | ||
|
|
||
| if (invocation_ctx) { | ||
| return *invocation_ctx; | ||
| } | ||
| return GetEvaluationContext(); | ||
| } |
There was a problem hiding this comment.
The current implementation of MergeContexts doesn't follow the OpenFeature specification for context merging. It should merge contexts from the invocation, client, and API (global) levels. This implementation only considers the invocation and client contexts, ignoring the global context available via GlobalContextManager. While EvaluationContext is not fully implemented, the logic here should at least be structured to reflect the specified merging hierarchy (invocation > client > global) as a placeholder.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>
Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>
This PR
ClientAPIclass which is the concrete implementation ofClient.Related Issues
Fixes #34