feat: Add EvaluationOptions structure#120
Conversation
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds C++ APIs for hook lifecycle callbacks, evaluation options, and typed flag evaluation details. Bazel targets expose the new components, while GoogleTest coverage validates construction, lifecycle behavior, hints, polymorphism, and supported value types. ChangesHook and Evaluation APIs
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant EvaluationContext
participant HookContext
participant Hook
participant FlagEvaluationDetails
HookContext->>Hook: Before with context and hints
Hook-->>EvaluationContext: optional updated context
HookContext->>Hook: After with evaluation details
HookContext->>Hook: Error with exception
HookContext->>Hook: Finally with evaluation details
FlagEvaluationDetails-->>Hook: flag key and resolution data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/evaluation_options_test.cpp (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<utility>forstd::move.
std::moveis used on line 112, which strictly requires the<utility>header. Although it compiles currently due to transitive includes from other standard library headers, it's best practice to explicitly include the required headers.♻️ Proposed fix
`#include` <any> `#include` <memory> `#include` <string> +#include <utility> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/evaluation_options_test.cpp` around lines 5 - 9, Add the standard <utility> header to the includes in evaluation_options_test.cpp so the std::move usage in the test is explicitly supported, without changing the surrounding test logic.openfeature/flag_evaluation_details.cpp (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
<utility>forstd::move.The code uses
std::moveextensively, which is defined in<utility>. While it may compile due to transitive inclusions from<string>or<optional>, it is best practice to include it explicitly.♻️ Proposed fix
`#include` <string> `#include` <string_view> +#include <utility> `#include` "openfeature/error_code.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openfeature/flag_evaluation_details.cpp` around lines 1 - 12, Update the includes in flag_evaluation_details.cpp to explicitly add the standard <utility> header required by the std::move usages, leaving the existing includes and implementation unchanged.openfeature/flag_evaluation_details.h (1)
25-26: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize constructor by taking
resolution_detailsby value.The wrapping constructor currently takes
ResolutionDetails<T>byconst&, which forces an internal copy of potentially expensive fields (like strings and the genericTmapping). Taking the argument by value and usingstd::moveallows the compiler to optimize out copies when an rvalue is passed.
openfeature/flag_evaluation_details.h#L25-L26: Change the parameter to take by value:ResolutionDetails<T> resolution_details.openfeature/flag_evaluation_details.cpp#L25-L29: Change the parameter to take by value and move it into the base constructor:: ResolutionDetails<T>(std::move(resolution_details)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openfeature/flag_evaluation_details.h` around lines 25 - 26, Update the FlagEvaluationDetails constructor in openfeature/flag_evaluation_details.h lines 25-26 to accept ResolutionDetails<T> by value, and update its definition in openfeature/flag_evaluation_details.cpp lines 25-29 to accept the same value parameter and move it into the ResolutionDetails<T> base constructor.openfeature/hook.h (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
const HookContext<T>&inBeforefor consistency.The
ctxparameter is passed by non-const reference inBefore, but byconstreference inAfter,Error, andFinally. Since any shared mutable state between stages should be managed internally (e.g. via a shared pointer toHookData),const HookContext<T>&is sufficient for all stages and provides a safer, consistent API contract.
openfeature/hook.h#L35-L36: Change the parameter toconst HookContext<T>& ctx.test/hook_test.cpp#L28-L29: Update the overriddenTrackingHook::Beforesignature to matchconst HookContext<T>& ctx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openfeature/hook.h` around lines 35 - 36, Change the Before method in openfeature/hook.h at lines 35-36 to accept const HookContext<T>&, matching the const contract used by After, Error, and Finally. Update the TrackingHook::Before override in test/hook_test.cpp at lines 28-29 to use the same const reference signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openfeature/flag_evaluation_details.cpp`:
- Around line 1-12: Update the includes in flag_evaluation_details.cpp to
explicitly add the standard <utility> header required by the std::move usages,
leaving the existing includes and implementation unchanged.
In `@openfeature/flag_evaluation_details.h`:
- Around line 25-26: Update the FlagEvaluationDetails constructor in
openfeature/flag_evaluation_details.h lines 25-26 to accept ResolutionDetails<T>
by value, and update its definition in openfeature/flag_evaluation_details.cpp
lines 25-29 to accept the same value parameter and move it into the
ResolutionDetails<T> base constructor.
In `@openfeature/hook.h`:
- Around line 35-36: Change the Before method in openfeature/hook.h at lines
35-36 to accept const HookContext<T>&, matching the const contract used by
After, Error, and Finally. Update the TrackingHook::Before override in
test/hook_test.cpp at lines 28-29 to use the same const reference signature.
In `@test/evaluation_options_test.cpp`:
- Around line 5-9: Add the standard <utility> header to the includes in
evaluation_options_test.cpp so the std::move usage in the test is explicitly
supported, without changing the surrounding test logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da405df6-b178-4523-a0ba-a41108057819
📒 Files selected for processing (11)
openfeature/BUILDopenfeature/base_hook.hopenfeature/evaluation_options.hopenfeature/flag_evaluation_details.cppopenfeature/flag_evaluation_details.hopenfeature/hook.cppopenfeature/hook.htest/BUILDtest/evaluation_options_test.cpptest/flag_evaluation_details_test.cpptest/hook_test.cpp
This PR
EvaluationOptionsstructure to enable per-invocation hook registration and evaluation hints according to the OpenFeature Hooks specification 4.5.1.EvaluationOptionsintest/evaluation_options_test.cppto ensure proper container initialization, ordering preservation, safe downcasting, hint storage, and copy/move semantics.Related Issues
Fixes #111