Skip to content
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

Rework EvaluationContext to use Structure type #53

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

benjiro
Copy link
Member

@benjiro benjiro commented Aug 27, 2022

Born from the discussions in #25 and open-feature/java-sdk#50

  • Adds Structure type which represents JSON data
  • Adds Value wrapper class that houses the supported types (Structure, List, bool, int, double, Value - note ints are stored and doubles, and rounded when returned)
  • EvaluationContext exposes methods to interact with its underlying keyvalue pairs of string, Value
  • Update interfaces to use Structure instead of generics

- Adds `Structure` type which represents JSON data
- Adds `Value` wrapper class that houses the supported types (Structure, List, bool, int, double, Value)
- `EvaluationContext` exposes methods to interact with its underlying keyvalue pairs of string, `Value`
- Update interfaces to use `Structure` instead of generics

Signed-off-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com>
@benjiro benjiro added the breaking-change Changes in exposed interfaces or behaviour label Aug 27, 2022
Accidentally removed coverlet.msbuild which is needed to codecov

Signed-off-by: Benjamin Evenson <2031163+benjiro@users.noreply.github.com>
@benjiro benjiro force-pushed the feat/improve-ec-interactions branch from f5b9c26 to 86c8cdb Compare August 27, 2022 03:59
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2022

Codecov Report

Merging #53 (86c8cdb) into main (1a596bf) will increase coverage by 93.56%.
The diff coverage is 78.64%.

@@            Coverage Diff            @@
##           main      #53       +/-   ##
=========================================
+ Coverage      0   93.56%   +93.56%     
=========================================
  Files         0       18       +18     
  Lines         0      404      +404     
  Branches      0       33       +33     
=========================================
+ Hits          0      378      +378     
- Misses        0       18       +18     
- Partials      0        8        +8     
Impacted Files Coverage Δ
src/OpenFeature.SDK/FeatureProvider.cs 100.00% <ø> (ø)
src/OpenFeature.SDK/NoOpProvider.cs 100.00% <ø> (ø)
src/OpenFeature.SDK/OpenFeatureClient.cs 99.21% <ø> (ø)
src/OpenFeature.SDK/Model/Value.cs 54.54% <54.54%> (ø)
src/OpenFeature.SDK/Model/Structure.cs 80.76% <80.76%> (ø)
src/OpenFeature.SDK/Model/EvaluationContext.cs 91.48% <93.10%> (ø)
src/OpenFeature.SDK/OpenFeature.cs 100.00% <0.00%> (ø)
src/OpenFeature.SDK/Model/Metadata.cs 100.00% <0.00%> (ø)
...ature.SDK/Extension/ResolutionDetailsExtensions.cs 100.00% <0.00%> (ø)
src/OpenFeature.SDK/Hook.cs 100.00% <0.00%> (ø)
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@benjiro benjiro changed the title Rework EvaluationContext to use Structure type [WIP] Rework EvaluationContext to use Structure type Aug 28, 2022
@toddbaert toddbaert self-assigned this Sep 2, 2022
@toddbaert
Copy link
Member

PSA: I'm going to bring this one home at the request of @benjiro (thanks for doing most of the work already!)

@toddbaert toddbaert self-requested a review September 2, 2022 20:05
@toddbaert toddbaert changed the title [WIP] Rework EvaluationContext to use Structure type Rework EvaluationContext to use Structure type Sep 2, 2022
Signed-off-by: Todd Baert <toddbaert@gmail.com>
@toddbaert toddbaert marked this pull request as ready for review September 2, 2022 20:30
@toddbaert
Copy link
Member

@kinyoklion I would like your take on this relevant issue if you have a moment, I've come to value your input 😅

Signed-off-by: Todd Baert <toddbaert@gmail.com>
@toddbaert
Copy link
Member

@benjiro @kinyoklion I've added changes analogous to open-feature/java-sdk#61 here as well.

Copy link
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me. Not sure if the empty XML comments are a problem.

@toddbaert
Copy link
Member

toddbaert commented Sep 7, 2022

Code looks good to me. Not sure if the empty XML comments are a problem.

I thought I got them all. Can you point out any I missed? @kinyoklion

EDIT: Oh, I see, some params are missed. I will get those. Thanks!

Copy link
Member Author

@benjiro benjiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏆 Thanks for finishing this up. Few params comments missing but looks fine otherwise.

@kinyoklion
Copy link
Member

Not a problem exactly, but this does allow for circular references. I assume that would be a "Just don't do that." Just thinking through if a provider would need to handle that situation.

@toddbaert
Copy link
Member

toddbaert commented Sep 8, 2022

Not a problem exactly, but this does allow for circular references. I assume that would be a "Just don't do that." Just thinking through if a provider would need to handle that situation.

Ya, good point. I've seen similar issues with some vendor's SDKs as well, especially converting circular objects to JSON in javascript. It might be worth noting somewhere, maybe even in our general documentation.

I will update the param comments and merge. I think we should delay a release for a while though, since I think we may have one more breaking change related to: open-feature/spec#138

Signed-off-by: Todd Baert <toddbaert@gmail.com>
@toddbaert toddbaert merged commit 01c0f34 into main Sep 8, 2022
@toddbaert
Copy link
Member

Not a problem exactly, but this does allow for circular references. I assume that would be a "Just don't do that." Just thinking through if a provider would need to handle that situation.

doc mention: open-feature/docs.openfeature.dev#110

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes in exposed interfaces or behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants