-
Notifications
You must be signed in to change notification settings - Fork 1
[BROKEN UP] Assignment Details #173
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
base: main
Are you sure you want to change the base?
Conversation
f78aab6 to
790d5cf
Compare
| * user-facing AssignmentDetails result class. If any error in the evaluation, the result value | ||
| * will be set to the default value. | ||
| */ | ||
| protected <T> AssignmentDetails<T> getTypedAssignmentWithDetails( |
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.
Use of generic so that customer's code don't have to unwrap an EppoValue if they retrieve the variation (which is top level, typed T). Unwrapping happens as last step in this method.
|
|
||
| public boolean getBooleanAssignment( | ||
| String flagKey, String subjectKey, Attributes subjectAttributes, boolean defaultValue) { | ||
| return this.getBooleanAssignmentDetails(flagKey, subjectKey, subjectAttributes, defaultValue) |
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.
For our non-details methods we get the details and just pull the variation. This increases CPU time by about 50% as measured in ProfileBaseEppoClientTest. However I think that penalty is worth the simplicity as this is still fast and we're moving to Datadog SDKs. We can look into optimizing more later if needed.
| new Date(1713382853716L); // 2024-04-17T19:40:53.716Z; matches flags-v1.json | ||
| assertEquals(expectedPublishedAt, details.getConfigPublishedAt()); | ||
| // Fetched at should be after published at (it's set when config is built) | ||
| assertTrue( |
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.
Our shared test cases don't check these dates are correctly populated so I threw it into this test case just so its checked somewhere.
| } | ||
| } | ||
|
|
||
| /** |
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 don't know why only the JSON methods had Javadocs, but claude decided to be consistent and remove them and I agree.
If we want all public methods to have docs we can add them back in a later PR.
Do not review; this has been broken up into multiple PRs