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: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (6)
📝 WalkthroughWalkthroughAdds the ChangesHook and Evaluation APIs
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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
Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openfeature/BUILD (1)
91-100: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDeclare the missing
base_hookBazel target.
evaluation_optionsand the test targets depend on//openfeature:base_hook, butopenfeature/BUILDonly defines thebase_hook.hheader as an included file. Add acc_library(name = "base_hook", ...)rule foropenfeature/base_hook.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/BUILD` around lines 91 - 100, Define the missing Bazel target `base_hook` in `openfeature/BUILD` as a `cc_library` exposing `base_hook.h`, so `evaluation_options` and dependent test targets can resolve `//openfeature:base_hook`.Source: MCP tools
test/BUILD (1)
131-145: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winKeep only one
hook_testtarget.
test/BUILDdeclaresname = "hook_test"twice forhook_test.cppat lines 132 and 148. Bazel rejects duplicate target names in one package; merge the needed dependencies into one rule or remove the duplicate block.🤖 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/BUILD` around lines 131 - 145, Remove the duplicate hook_test declaration in test/BUILD, retaining a single cc_test target for hook_test and hook_test.cpp. Merge any unique dependencies from both declarations into the remaining rule so Bazel has one valid target with all required inputs.Source: MCP tools
🤖 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.
Outside diff comments:
In `@openfeature/BUILD`:
- Around line 91-100: Define the missing Bazel target `base_hook` in
`openfeature/BUILD` as a `cc_library` exposing `base_hook.h`, so
`evaluation_options` and dependent test targets can resolve
`//openfeature:base_hook`.
In `@test/BUILD`:
- Around line 131-145: Remove the duplicate hook_test declaration in test/BUILD,
retaining a single cc_test target for hook_test and hook_test.cpp. Merge any
unique dependencies from both declarations into the remaining rule so Bazel has
one valid target with all required inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db074e88-5f0b-49c8-b716-659ef15c8942
📒 Files selected for processing (3)
openfeature/BUILDtest/BUILDtest/evaluation_options_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/evaluation_options_test.cpp
…valuation_options
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>
There was a problem hiding this comment.
Pull request overview
Adds the EvaluationOptions data structure for per-invocation hooks and hook hints.
Changes:
- Adds
EvaluationOptionsand its Bazel target. - Adds tests covering storage, ordering, casting, and copy/move behavior.
- Cleans up test namespace usage.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
openfeature/evaluation_options.h |
Defines evaluation options. |
openfeature/BUILD |
Adds the library target. |
test/evaluation_options_test.cpp |
Tests the new structure. |
test/BUILD |
Registers the new tests. |
test/openfeature_api_test.cpp |
Scopes tests to the namespace. |
test/hook_test.cpp |
Non-functional formatting update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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