search

open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
62 stars 17 forks source link

chore: revert breaking setProvider #190

Closed toddbaert closed 7 months ago

toddbaert commented 8 months ago

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 https://github.com/open-feature/dotnet-sdk/pull/184. I like the idea of https://github.com/open-feature/dotnet-sdk/pull/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 like init()/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.

codecov[bot] commented 8 months ago

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (26cd5cd) 93.86% compared to head (528b723) 93.50%.

:exclamation: Current head 528b723 differs from pull request most recent head a9d9fbc. Consider uploading reports for the commit a9d9fbc to get more accurate results

Files Patch % Lines
src/OpenFeature/Api.cs 50.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lukas-reining commented 8 months ago

I like the idea of https://github.com/open-feature/dotnet-sdk/pull/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 like init()/shutdown()?, and what should providers do?).

I like the idea @toddbaert, and I think we could also add them later without even having a breaking change. I think especially for getting flags we should later add cancellationTokens but maybe as you said with first discussing all the possible side effects.

toddbaert commented 8 months ago

@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 ValueTask as you proposed.

@lukas-reining @askpt @kinyoklion @benjiro @bacherfl

austindrenski commented 8 months ago

@toddbaert wrote (https://github.com/open-feature/dotnet-sdk/pull/190#issuecomment-1899033139):

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 ValueTask as you proposed.

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.

toddbaert commented 8 months ago

@toddbaert wrote (#190 (comment)):

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 ValueTask as you proposed.

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.

I will take a look, this is the sort of thing I'd like to have resolved in this release. :pray:

toddbaert commented 7 months ago

There was some fun pseudo-conflicts on this branch after recent merges, but I think we should be good now.

toddbaert commented 7 months ago

One more cleanup because the UI is clearly having some issues this morning, but otherwise LGTM (again)

At least we can be reasonably satisfied the tests are stable now, they've run like 30 times this morning due to this PR :joy: