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!: re-design No-op Provider #56

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

sheepduke
Copy link
Contributor

This PR

  • Change the implementation of NoOpProvider.
    • Always return EvaluationError with ProviderNotReady code.
    • Remove the ability to change its behaviors via constructors, EvaluationContext etc.
  • Add mockall dependency to mock FeatureProvider trait.
  • Adjust usage of TypedBuilder.
    • Remove typed builder for EvaluationContext.
      => Since every field is optional so just use EvaluationContext::default().with_xx() to make it more unified.
    • Remove typed builder usage for some simple types. => Less code bloat.
  • Adjust release-please pipeline.
    • Check format before building it. => Shorten pipeline build time when format is wrong.
    • Move "deny warnings" from lib.rs to build pipeline. => Better DX.

Related Issues

Notes

The main purpose of changing NoOpProvider is to make it more compliant with the spec. The spec mentioned that:

In the absence of a provider the evaluation API uses the "No-op provider", which simply returns the supplied default flag value.
The default NoOpProvider should return the supplied default flag value.

In Rust we leverage the type system to model this, instead of letting user provide a default value upon function invocation. When the flag evaluation fails due to any reason, an EvaluationError is returned, and the caller can do something like client.get_i32_value("flag_key", &context, None).unwrap_or(100) to provide a default value.

Now the NoOpProvider returns the default value of each data type if not configured. This is actually incorrect because the unwrap_or(..) part will never be executed (because it is always Ok) and the caller cannot chain a default value then.

Follow-up Tasks

How to test

@beeme1mr
Copy link
Member

beeme1mr commented Jan 9, 2024

I am adding @gruebel because he expressed interest in getting involved with the Rust SDK.

src/api/client.rs Outdated Show resolved Hide resolved
src/api/client.rs Show resolved Hide resolved
@beeme1mr
Copy link
Member

beeme1mr commented Jan 9, 2024

@sheepduke is the basic usage example on the readme still accurate? If I understand correctly, the fallback value is defined using .unwrap_or(false) instead of including it as an argument.

Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice work 🏅

Cargo.toml Show resolved Hide resolved
src/evaluation/context.rs Show resolved Hide resolved
sheepduke and others added 2 commits January 10, 2024 18:15
Co-authored-by: Justin Abrahms <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
@sheepduke
Copy link
Contributor Author

@sheepduke is the basic usage example on the readme still accurate? If I understand correctly, the fallback value is defined using .unwrap_or(false) instead of including it as an argument.

Oh!!!! My apologies.

Added it. Really appreciate your reminder!

Signed-off-by: Daian YUE 🐏 <[email protected]>
@sheepduke sheepduke merged commit 554cf22 into open-feature:main Jan 11, 2024
3 checks passed
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.

4 participants