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

feat: add integer and float specific resolver methods #124

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

mschoenlaub
Copy link
Contributor

@mschoenlaub mschoenlaub commented Apr 16, 2024

This PR

  • adds support for Conditional Requirement 1.3.3.1 to the client, as well as the included providers
  • modifies the InMemoryProvider to work slightly better with subclasses

Related Issues

Fixes #121

Notes

It's somewhat weird that the client's tests use an actual NoopProvider instead of a mock. That kind of couples these changes more than i'd like to. But that's just how it is for now :)
I'm also wondering, if we wouldn't actually need a slightly more elaborated solution where the client falls back to the fetch_number method on the rovider, if it doesn't have a fetch_integer/fetch_float. The reason is, that currently the client blindly delegates these calls, and there might be providers out in the wild.

@mschoenlaub mschoenlaub requested a review from a team as a code owner April 16, 2024 19:16
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.48%. Comparing base (a6c789d) to head (af3cdb8).

❗ Current head af3cdb8 differs from pull request most recent head 07c0341. Consider uploading reports for the commit 07c0341 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   99.46%   99.48%   +0.02%     
==========================================
  Files          15       15              
  Lines         187      195       +8     
==========================================
+ Hits          186      194       +8     
  Misses          1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@maxveldink maxveldink left a comment

Choose a reason for hiding this comment

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

🦑 Nice! Looks good to me, thank you for implementing this!

One thing this prompted me to think about (which is out of scope for this PR), is that I think the type mismatch check actually belongs in the client (according to Requirement 1.3.4, instead of the InMemoryProvider. We should guarantee this behavior at the client, as I don't see anywhere in the Provider side of the spec where that guarantee is a requirement.

Thoughts on that? I can turn it into an issue to improve that.

@mschoenlaub
Copy link
Contributor Author

@maxveldink , I definitely agree with your interpretation of the spec.
It would also make a bunch of stuff easier, like fallback for optional methods and not having the same type checking code in every provider.

@maxveldink
Copy link
Member

Cool, I've added #126 to track that work (hopefully I can get to it soon). I think I'll go ahead and merge this in and cut a new release so we can get this behavior released.

@maxveldink maxveldink merged commit eea9d17 into open-feature:main Apr 22, 2024
8 checks passed
@toddbaert
Copy link
Member

I was late to this review, but it all looks right to me from a spec perspective. Nice job and thanks @mschoenlaub and @maxveldink

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement float and integer variations on number type
3 participants