Skip to content

Conversation

emyller
Copy link

@emyller emyller commented Sep 18, 2025

Warning

Because we decided to break this project into smaller pieces only after work started, this branch will only see the engine changes.

Contributes to #98

This is pass 1 of 3.

  1. Implement get_evaluation_result + engine tests passing. No deleted code. (this PR)
  2. Use the new engine in the client local evaluation. Minimal changes. (PR)
  3. Delete unused code. Minimal refactor. (PR)

The engine work is largely inspired by flagsmith-engine.

Note

Notes are added to call for discussion or follow up.

@emyller emyller force-pushed the feat/context-values branch 6 times, most recently from 8a8c07a to 3f2eaf4 Compare September 24, 2025 23:22
@emyller emyller force-pushed the feat/context-values branch from 3f2eaf4 to 7295ca8 Compare September 24, 2025 23:25
@emyller emyller self-assigned this Sep 24, 2025
@emyller emyller force-pushed the feat/context-values branch from 38c5cef to fe68922 Compare September 26, 2025 19:44
@emyller emyller changed the base branch from main to feat/context-values-umbrella October 2, 2025 01:43
@emyller emyller force-pushed the feat/context-values branch from 4e231b6 to cc6ae4c Compare October 2, 2025 01:55
@emyller emyller marked this pull request as ready for review October 2, 2025 01:57
@emyller emyller requested a review from a team as a code owner October 2, 2025 01:57
@emyller emyller requested review from gagantrivedi and removed request for a team October 2, 2025 01:57
@khvn26 khvn26 self-requested a review October 2, 2025 08:52
Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

Looks great overall — just a handful of comments, some of them relevant to Flagsmith/engine-test-data#14.

.gitmodules Outdated
path = tests/Engine/EngineTests/EngineTestData
url = [email protected]:Flagsmith/engine-test-data.git
branch = v1.0.0
branch = feat/context-values
Copy link
Member

Choose a reason for hiding this comment

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

This should be updated to include the new tests

Copy link
Author

Choose a reason for hiding this comment

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

Done in 0a201c6! 🎉

@emyller emyller force-pushed the feat/context-values branch from 842a88f to d9d977c Compare October 11, 2025 01:19
(cherry picked from commit 2913611)
@emyller emyller force-pushed the feat/context-values branch from fc10655 to 7d0d026 Compare October 14, 2025 19:38
Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

With the current test suite passing, I'm confident to approve this.

Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

Sorry, one last comment: we need to implement https://github.com/Flagsmith/engine-test-data/pull/28/files.

@emyller emyller force-pushed the feat/context-values branch from 0e5a777 to acc06b3 Compare October 14, 2025 22:13
@emyller
Copy link
Author

emyller commented Oct 14, 2025

Sorry, one last comment: we need to implement https://github.com/Flagsmith/engine-test-data/pull/28/files.

Addressed in acc06b3. The client side also follows with 4145960.

@emyller emyller requested a review from khvn26 October 14, 2025 22:21
path = tests/Engine/EngineTests/EngineTestData
url = [email protected]:Flagsmith/engine-test-data.git
branch = v1.0.0
tag = v2.3.0
Copy link
Member

@khvn26 khvn26 Oct 15, 2025

Choose a reason for hiding this comment

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

Let's include the variant sorting tests:

Suggested change
tag = v2.3.0
tag = v2.4.0

Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

One tiny comment and we're good to go.

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.

3 participants