Skip to content

feat: Add EvaluationOptions structure - #120

Open
NeaguGeorgiana23 wants to merge 32 commits into
mainfrom
evaluation_options
Open

feat: Add EvaluationOptions structure#120
NeaguGeorgiana23 wants to merge 32 commits into
mainfrom
evaluation_options

Conversation

@NeaguGeorgiana23

Copy link
Copy Markdown
Contributor

This PR

  • Introduces the EvaluationOptions structure to enable per-invocation hook registration and evaluation hints according to the OpenFeature Hooks specification 4.5.1.
  • Adds unit tests for EvaluationOptions in test/evaluation_options_test.cpp to ensure proper container initialization, ordering preservation, safe downcasting, hint storage, and copy/move semantics.

Related Issues

Fixes #111

NeaguGeorgiana23 and others added 23 commits July 7, 2026 13:19
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>
@NeaguGeorgiana23
NeaguGeorgiana23 requested review from a team as code owners July 15, 2026 12:25
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@NeaguGeorgiana23, you've reached your PR review limit, so we couldn't start this review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0e4c50c2-9989-45b5-9979-42c403fabcb4

📥 Commits

Reviewing files that changed from the base of the PR and between 12c6b9a and 5bc9b5a.

📒 Files selected for processing (6)
  • openfeature/BUILD
  • openfeature/evaluation_options.h
  • test/BUILD
  • test/evaluation_options_test.cpp
  • test/hook_test.cpp
  • test/openfeature_api_test.cpp
📝 Walkthrough

Walkthrough

Adds the BaseHook abstraction and EvaluationOptions structure. Wires the new library and test targets in Bazel. Adds tests for hook storage, downcasting, hints, and copy/move construction.

Changes

Hook and Evaluation APIs

Layer / File(s) Summary
Evaluation options API and Bazel wiring
openfeature/base_hook.h, openfeature/evaluation_options.h, openfeature/BUILD
Defines non-copyable polymorphic BaseHook and EvaluationOptions with hook storage and HookHints. Adds the corresponding Bazel library target.
Evaluation options validation
test/BUILD, test/evaluation_options_test.cpp
Adds test targets and verifies default state, insertion order, dynamic casts, heterogeneous hints, and copy/move construction.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: oxddr

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements EvaluationOptions but does not implement the remaining Hook data structures requested by issue #111. Implement Hook data and Hook context, or update the issue linkage and description to reflect the partial scope.
Docstring Coverage ⚠️ Warning Docstring coverage is 14.71% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: adding the EvaluationOptions structure.
Description check ✅ Passed The description accurately explains the EvaluationOptions implementation and its test coverage.
Out of Scope Changes check ✅ Passed The source, build, and test changes support the EvaluationOptions objective and do not introduce unrelated changes.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@NeaguGeorgiana23 NeaguGeorgiana23 changed the title Evaluation options feat: Add EvaluationOptions structure Jul 15, 2026
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
test/evaluation_options_test.cpp (1)

5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Include <utility> for std::move.

std::move is 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 value

Include <utility> for std::move.

The code uses std::move extensively, 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 win

Optimize constructor by taking resolution_details by value.

The wrapping constructor currently takes ResolutionDetails<T> by const&, which forces an internal copy of potentially expensive fields (like strings and the generic T mapping). Taking the argument by value and using std::move allows 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 win

Use const HookContext<T>& in Before for consistency.

The ctx parameter is passed by non-const reference in Before, but by const reference in After, Error, and Finally. Since any shared mutable state between stages should be managed internally (e.g. via a shared pointer to HookData), const HookContext<T>& is sufficient for all stages and provides a safer, consistent API contract.

  • openfeature/hook.h#L35-L36: Change the parameter to const HookContext<T>& ctx.
  • test/hook_test.cpp#L28-L29: Update the overridden TrackingHook::Before signature to match const 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

📥 Commits

Reviewing files that changed from the base of the PR and between b7e65b5 and 06bd51c.

📒 Files selected for processing (11)
  • openfeature/BUILD
  • openfeature/base_hook.h
  • openfeature/evaluation_options.h
  • openfeature/flag_evaluation_details.cpp
  • openfeature/flag_evaluation_details.h
  • openfeature/hook.cpp
  • openfeature/hook.h
  • test/BUILD
  • test/evaluation_options_test.cpp
  • test/flag_evaluation_details_test.cpp
  • test/hook_test.cpp

@NeaguGeorgiana23
NeaguGeorgiana23 requested a review from oxddr July 22, 2026 12:43
@coderabbitai coderabbitai Bot mentioned this pull request Jul 28, 2026
Signed-off-by: NeaguGeorgiana23 <115723925+NeaguGeorgiana23@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Declare the missing base_hook Bazel target.

evaluation_options and the test targets depend on //openfeature:base_hook, but openfeature/BUILD only defines the base_hook.h header as an included file. Add a cc_library(name = "base_hook", ...) rule for openfeature/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 win

Keep only one hook_test target.

test/BUILD declares name = "hook_test" twice for hook_test.cpp at 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

📥 Commits

Reviewing files that changed from the base of the PR and between 06bd51c and 12c6b9a.

📒 Files selected for processing (3)
  • openfeature/BUILD
  • test/BUILD
  • test/evaluation_options_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/evaluation_options_test.cpp

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>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds the EvaluationOptions data structure for per-invocation hooks and hook hints.

Changes:

  • Adds EvaluationOptions and 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.

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.

Hook data structures

2 participants