-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Context Values support, GetEvaluationResult
#171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
emyller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The engine code makes sense! There are only a few minor points I think need visiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the first phase of a new engine evaluation system that supports Context Values with JSONPath-based evaluation. It introduces GetEvaluationResult method and the ContextEvaluator engine, while maintaining compatibility with the existing engine implementation.
Key changes:
- New
ContextEvaluatorengine with Context Values support via JSONPath expressions - Auto-generated data models from JSON Schema for
EvaluationContextandEvaluationResult - Temporary infrastructure for testing against engine-test-data revisions outside the submodule
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds build tooling for code generation from JSON schemas and temporary engine test data cloning |
| Flagsmith.EngineTest/EngineTestV2.cs | New test suite for the Context Values engine using engine-test-data v2 |
| Flagsmith.EngineTest/EngineTestData | Updates submodule commit reference |
| Flagsmith.EngineTest/EngineTest.csproj | Bumps xunit to 2.4.2 for Assert.Equivalent support |
| Flagsmith.Engine/Segment/Evaluator.cs | Implements ContextEvaluator with JSONPath support and improves type conversion safety |
| Flagsmith.Engine/Segment/Constants.cs | Adds DefaultPriority constant for segment override priority handling |
| Flagsmith.Engine/Interfaces/IEngine.cs | Adds GetEvaluationResult interface method |
| Flagsmith.Engine/EvaluationResult/EvaluationResult.cs | Auto-generated result classes from JSON Schema |
| Flagsmith.Engine/EvaluationContext/EvaluationContext.cs | Auto-generated context classes from JSON Schema |
| Flagsmith.Engine/Engine.cs | Implements GetEvaluationResult and Identity.Key enrichment |
| Flagsmith.Client.Test/FlagsmithTest.cs | Skips flaky analytics test |
| Flagsmith.Client.Test/Fixtures.cs | Code formatting cleanup |
| .gitmodules | Changes submodule branch reference to tag |
| .gitignore | Adds EngineTestDataV2 to ignored paths |
| .github/workflows/formatting-and-tests.yml | Adds .NET 10.x testing and engine-test-data v2 cloning step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Evandro Myller <22429+emyller@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
emyller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only pending a non-blocking question here. Otherwise LGTM.
Closes #166.
This is the first in a series of 3 PRs:
In this PR, we:
ContextEvaluatorengine that takes JSONPath-based Context Values into account, covered by latest engine-test-data. The type-coerced evaluation part is reused from the old (current) engine implementation and will be migrated in PR 3/3.Assert.Equivalent.TestAnalyticsDataConsistencyWithConcurrentCallsToGetFlagsis flaky #170.