-
Notifications
You must be signed in to change notification settings - Fork 40
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
add reasons, table #140
add reasons, table #140
Conversation
I am not sure that I see strong benefit in enumerating them. It really depends on what the use case for the reason is seen to be at all. If the reason is to help a developer understand why they got the value they got, so they can presumably debug it, then the more specific it can be the better. Different providers have different pipelines and can be more specific about the part of their pipelines involved in getting a result. If we expect someone to be able to respond in some dynamic way to these conditions, then it would make sense for it to be enumerated. |
@kinyoklion Agreed. I think the reason is most valuable in hooks that do telemetry/logging and for debugging purposes. I'd not expect people to write logic around particular reasons. This is why the spec was less prescriptive before. However, SDKs have implemented these are enums, so I was happy to button up the inconsistency. Let's see what others say. This is a relatively minor issue but I'd like to get it remedied as we head toward a possible 1.0 of some SDKs. |
I would suggest to make it more open. I will suggest to have a string field instead. |
I think that for consistency it's nice to have this set of standard reasons. But I think it's important for this standard list to be extended by providers if necessary if there is another 'reason' that has not been thought of beforehand (it also allows for adding an new reason to the list without having to re-release the SDKs with a breaking change. So I agree that the reason should be implemented as a However, I see value in providing these 'standard' reasons as a const defined in the openfeature SDKs. Then the provider SDKs can just use these constants (and also getting consistency across providers) rather than having to re-define them in for every provider (or worse, hard-coding the same string literal throughout the codebase). |
I will update this PR. I think the consensus is to have an extensible set pre-defined strings for
|
Signed-off-by: Todd Baert <[email protected]>
@agentgonzo @thomaspoignant @kinyoklion I've updated the PR. Please re-review. |
Many SDKs have implemented these reasons as enums or limited strings, they are not free-form. I also think we can be reasonably confident most evaluation reasons can be bucketed into one of these.This PR:adds thereasons
previously in a non-normative section into a normative sectionadds a table explaining each reasonIf we think that reasons should be free-form, we should make changes in SDKs to support this. If you think a reason is missing, feel free to add it or make a case in a comment.Reasons
. See the thread for more reasoning, but TLDR:Reaons
; they are for logging/debugging/telemetry