-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: implemented tracking as per spec #1228
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1228 +/- ##
============================================
+ Coverage 93.07% 93.65% +0.58%
- Complexity 441 465 +24
============================================
Files 41 43 +2
Lines 1025 1088 +63
Branches 85 86 +1
============================================
+ Hits 954 1019 +65
+ Misses 44 43 -1
+ Partials 27 26 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thank you the changes look good, but please remove the start imports, and (personal preference) use assertJ for assertions. Plus the comment i wrote regarding the assertCode
7831008
to
d40050f
Compare
I will wait for a few more days, to give other @open-feature/sdk-java-maintainers the possibility to respond before merging this. Thank you for the contribution |
Will review tomorrow! |
I will also have a look tomorrow if I find time :) |
src/main/java/dev/openfeature/sdk/MutableTrackingEventDetails.java
Outdated
Show resolved
Hide resolved
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.
Defintely on the right track, please see:
- https://github.com/open-feature/java-sdk/pull/1228/files#r1860724481
- https://github.com/open-feature/java-sdk/pull/1228/files#r1860735063
In particular. These are blockers from my POV.
92a1d6c
to
fbef251
Compare
fbef251
to
1131417
Compare
d4b244f
to
9987185
Compare
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.
Approving - I'd love to hear from @justinabrahms before merging though.
Also @warber and other reviewers, please consider this; cc @aepfli @lukas-reining @thomaspoignant @kinyoklion I'm particularly interested in your take on this.
src/test/java/dev/openfeature/sdk/MutableTrackingEventDetailsTest.java
Outdated
Show resolved
Hide resolved
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.
Looks good to me!
I only have one concerns about the numeric value.
src/main/java/dev/openfeature/sdk/ImmutableTrackingEventDetails.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
Signed-off-by: Bernd Warmuth <bernd.warmuth@dynatrace.com>
f4eb3e9
to
1ecffec
Compare
Since this is an SDK change, I'ld like to get a few more approvals before merge. |
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.
Looks great! Thanks!
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Quality Gate passedIssues Measures |
This PR
Related Issues
Fixes #1164