open-feature / python-sdk

Python SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
48 stars 16 forks source link

FlagResolutionDetails reason field should accept any string #262

Closed keelerm84 closed 7 months ago

keelerm84 commented 7 months ago

The specification for resolution details states that the reason field should be an optional string. There are some pre-defined reason values (reflected in the Reason enum), but a provider should be free to provide an arbitrary reason.

I believe the FlagResolutionDetails.reason field should be updated to store an Optional[str] instead of the current Optional[Reason].

Happy to work on the PR if this seems acceptable.

See the related issue on the SDK spec.

beeme1mr commented 7 months ago

This relates to this issue recently opened in the spec. https://github.com/open-feature/spec/issues/236

gruebel commented 7 months ago

Sounds reasonable, but I would prefer to change it to Optional[Union[str, Reason]] if possible. Then you indicate the user about an existing set of reasons, but still you can choose to add your own.

federicobond commented 7 months ago

Good catch @keelerm84!

Reason is a str, so Optional[Union[str, Reason]] describes the same set as Optional[str]. Personally, I am more inclined to the latter because Union[str, Reason] gives the impression that there are values from Reason that are not included in the set of str and should be handled in a special way. I would rather users think of them as str's all the way.

gruebel commented 7 months ago

My thinking was more about discoverability, because if it is set to str, then you wouldn't know about the predefined reasons and worst write your own.

If this will be adjusted, then we should also change the typing for FlagEvaluationDetails.

beeme1mr commented 7 months ago

My thinking was more about discoverability, because if it is set to str, then you wouldn't know about the predefined reasons and worst write your own.

That's basically what we do in the JS SDK. You can see an example here.