-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Implement Context values for Segments #8
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
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Required to merge Flagsmith/engine-test-data#8.
Co-authored-by: Kim Gustyr <[email protected]>
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.
Yet to dig into the details of the tests but out of my mind I have 3 comments:
- Can we come up with human filenames ? To avoid redundancy because of not getting where to look at
- I see here the common usage anticipated, would it be worth to have a pre.commit script that aggregates the test into one
index.json? - And a mix of my 2 above comments, keep the files separately accessibles but with an understandable name (cf here)
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.
I mostly agree with @Zaimwa9, and honestly don't see actual improvements to existing test data.
That said, I'm approving because improving names might be out of scope here. This work should already unblock moving forward with adding new test cases.
Created #17 to address this. I agree with @emyller that further modifications should be out of scope of this PR; here, we solve transforming existing test data, and adding one test case for context values. |
Contributes to Flagsmith/flagsmith-engine#219.
Closes #11.
In this PR, we:
I've decided to use a JSONSchema spec for the test case data so it's self-documented and easily consumed.
We should release v2.0.0 once this is merged.