-
Notifications
You must be signed in to change notification settings - Fork 19
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: revert breaking setProvider #190
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
- Coverage 93.86% 93.50% -0.37%
==========================================
Files 23 23
Lines 946 954 +8
Branches 105 105
==========================================
+ Hits 888 892 +4
- Misses 34 38 +4
Partials 24 24 ☔ View full report in Codecov by Sentry. |
07c092b
to
35f806a
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.
Nice, should provide a gentler migration path downstream.
Couple of suggestions, including a while-we're-at-it related to #185 🤞
I like the idea @toddbaert, and I think we could also add them later without even having a breaking change. |
@austindrenski I've accepted a few suggestions, and noted that others, which I did not accept are corollaries to this discussion and don't apply if we go with the route proposed there. Please re-review. Based on the slack discussion, it sounds like we'd like to go the route of merging this to release a non-breaking version, and then subsequently working on a breaking release with a bunch of async changes, including moving some methods over to |
cdc5153
to
1ae7b1c
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.
LGTM 🚢🚢🚢🚢
@toddbaert wrote (#190 (comment)):
Sounds good, but would like to flag #186 for consideration before we cut the non-breaking release. I opened #188 with a proposed solution, but want to call out that I drafted it with the assumption that the next release would be a breaking release, so it would help to get some fresh eyes on it. The downside to not fixing #186 before the next release is that shutdown/restart will remain broken, but since its already broken, it's not a showstopper if we need to punt. |
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.
LGTM! Unfortunately, our test coverage decreased due to the new methods being introduced.
I will take a look, this is the sort of thing I'd like to have resolved in this release. 🙏 |
ca3ac87
to
c6657f6
Compare
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
c6657f6
to
d34331a
Compare
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
There was some fun pseudo-conflicts on this branch after recent merges, but I think we should be good now. |
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.
Little follow-up cleanup because GitHub got weird applying those suggestions
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[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.
One more cleanup because the UI is clearly having some issues this morning, but otherwise LGTM (again)
Co-authored-by: Austin Drenski <[email protected]> Signed-off-by: Todd Baert <[email protected]>
At least we can be reasonably satisfied the tests are stable now, they've run like 30 times this morning due to this PR 😂 |
This PR makes the breaking change here non breaking, by adding a new method instead. This means our pending release will no longer be breaking.
This will allow us to release all the pending .NET work, without merging #184. I like the idea of #184, but I think I'd like us all to take some time to discuss exactly what we expect the
cancellationToken
to do in all circumstances (what should the SDK do on SDK methods likeinit()/shutdown()
?, and what should providers do?).I think buys us some without sacrificing too much, and we can release a 2.0.0 in the coming weeks.
Interested to hear the opinion of others.