-
Notifications
You must be signed in to change notification settings - Fork 39
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
Have different methods for float and int #116
Have different methods for float and int #116
Conversation
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree fully with the concept, thanks for noticing! I left some comments on structure, and I don't think we want to lose the description of the params for the flag evaluation methods; we can just merge the typed/untyped requirements, while keeping the params:
with parameters
flag key
(string, required),default value
(boolean | number | string | structure, required),evaluation context
(optional), andevaluation options
(optional),
Added a few more SDK devs to the PR as well.
@justinabrahms @agentgonzo @InTheCloudDan pinging you all on this one, since it's relevant to Java, and also the Cloudbees SDK differentiates between floats/ints, while the LD SDK uses an abstraction that can be interpreted as either. Justin suggested we use something more generic, like Number, which I'm also OK with, but I think in that case we still might want to add something to the spec as @thomaspoignant suggests to make sure we have some way of support both floating point and integer values. I think personally, I would prefer to just provide separate methods as @thomaspoignant has suggested here, but I'm interested in other takes... It does feel very "go-lang-y" which is why I think it was implemented this way in go-feature-flag. |
I'm not opposed to separating floats vs ints. I'd just like to avoid it if we could. But I'm hearing that maybe we can't? So... let's not. :) |
Signed-off-by: Thomas Poignant <[email protected]>
d17dba5
to
057a80f
Compare
Signed-off-by: Thomas Poignant <[email protected]>
This sounds fine to me, do we need to be more explicit about types so it's not left up to interpretation? Eg Thoughts? @toddbaert @thomaspoignant |
It is a bit too detailed from my point of view. |
Agreed. We also mention this, in the
https://github.com/open-feature/spec/blob/main/specification/types.md#number @thomaspoignant we might want to link to this after the normative spec text, maybe below your example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we're in agreement. LGTM assuming we clarify the language around "all types".
Signed-off-by: Thomas Poignant <[email protected]>
BTW the ci is failing on this link |
Re-ran it, seems to work now. Probably the site was slow for a time. |
Signed-off-by: Thomas Poignant <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me too. It's a bummer we need it but not every language can be as great as JavaScript! 😄
Anyone want to add a final approval for this? Seems like we have consensus. |
Signed-off-by: Thomas Poignant <[email protected]>
26c804f
to
505c263
Compare
See #115 for the context.
Discussion also on slack.