-
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
chore: improve contrib guide #863
chore: improve contrib guide #863
Conversation
ebb8c58
to
441ced2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
============================================
+ Coverage 95.30% 95.53% +0.23%
- Complexity 370 371 +1
============================================
Files 34 34
Lines 852 852
Branches 52 52
============================================
+ Hits 812 814 +2
+ Misses 21 20 -1
+ Partials 19 18 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
eeeb1c5
to
0fc710a
Compare
* Create an OpenFeature client. For internal use only. | ||
* | ||
* @param openFeatureAPI Backing global singleton | ||
* @param name Name of the client (used by observability tools). | ||
* @param version Version of the client (used by observability tools). | ||
* @deprecated Do not use this constructor. It's for internal use only. | ||
* Clients created using it will not run event handlers. | ||
* Use the OpenFeatureAPI's getClient factory method instead. | ||
*/ | ||
@Deprecated() // TODO: eventually we will make this non-public | ||
public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String name, String version) { | ||
OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String name, String version) { |
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.
This has bee deprecated for a while, but technically is a breaking change. I think we need to wait for a 2.0 for this.
@thomaspoignant @lukas-reining @kinyoklion is there any reason we should make this an exception?
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.
My expectation would be to wait until 2.0. We should probably be tracking this with an issue and "breaking-change" or other label (or project/mileston). So when we want to do 2.0 we can go through that list.
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.
Thanks @toddbaert @kinyoklion for feedback. I will document this and revert the change
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.
Created tracking issue #872 and reverted this change
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.
May want to update PR title. Aside from that LGTM.
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.
@kinyoklion ahh good one, thanks 👍
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.
Blocking for now for this.
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
Signed-off-by: Kavindu Dodanduwa <[email protected]>
4b86428
to
fa60f75
Compare
Quality Gate passedIssues Measures |
This PR
Fixes #858
and removes a long-running deprecation noticeOpenFeatureClient
constructor.