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: implement evaluation details and re-design evaluation API #24

Merged
merged 12 commits into from
Sep 27, 2023

Conversation

sheepduke
Copy link
Contributor

@sheepduke sheepduke commented Sep 21, 2023

This PR

  • Implement evaluation details get_xx_details for the client.
  • Re-designed the API to return Result<T, EvaluationError> and Result<EvaluationDetails<T>, EvaluationError> for get_xx_value and get_xx_details respectively.
  • Remove default_value parameter from the evaluation functions.
  • Add tests for all the spec requirements already supported.
  • Enhance README to reflect current change.
  • Add Apache 2 license file.
  • Eliminates all the warnings.

Related Issues

Notes

Result<T, E> is a powerful tool to build abstractions in Rust. By utilizing this instead of putting everything into one huge struct, we gain the following merits:

  • The spec indicates that default_value should always be returned when abnormal execution. Meaning its value is passed into each function and then (conditionally) returned back. It does not make much sense because caller could simply write client.get_int_value(..).unwrap_or(123) to provide 123 as a default value. There is no need to put it into the function parameter and in the return value.

  • The user could more easily decide what to do when the evaluation fails. Result provides many helper functions like map, unwrap_or etc.

  • Success cases and failure cases are strictly distinguished with 2 branches.

  • The abstractions and APIs become cleaner.

Follow-up Tasks

  1. Release 0.1.0.
  2. Implement hooks and events.
  3. Address other issues.

How to test

UT.

Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
@beeme1mr
Copy link
Member

This pr is massive 😅. I'll take a look tomorrow.

@sheepduke
Copy link
Contributor Author

sheepduke commented Sep 22, 2023

This pr is massive 😅. I'll take a look tomorrow.

Totally understood. Thanks for helping review it! :-D

I wanted to make it small, but the changes to the core evaluation abstractions brought too many changes...

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/api/api.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/api/api.rs Outdated Show resolved Hide resolved
src/provider/no_op_provider.rs Outdated Show resolved Hide resolved
sheepduke and others added 5 commits September 24, 2023 17:09
Co-authored-by: Michael Beemer <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Co-authored-by: Justin Abrahms <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Co-authored-by: Justin Abrahms <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
Signed-off-by: YUE Daian <[email protected]>
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@sheepduke sheepduke merged commit d6aace1 into open-feature:main Sep 27, 2023
2 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.

3 participants