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

Have different methods for float and int #116

Merged

Conversation

thomaspoignant
Copy link
Member

See #115 for the context.
Discussion also on slack.

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant thomaspoignant changed the title spec flag evaluation float int types Have different methods for float and int Jul 23, 2022
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Copy link
Member

@toddbaert toddbaert left a 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), and evaluation options (optional),

Added a few more SDK devs to the PR as well.

@toddbaert
Copy link
Member

toddbaert commented Jul 24, 2022

@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.

@justinabrahms
Copy link
Member

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 <thomas.poignant@gofeatureflag.org>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@benjiro
Copy link
Member

benjiro commented Jul 24, 2022

This sounds fine to me, do we need to be more explicit about types so it's not left up to interpretation? Eg
float vs float64(double) vs float128(decimal)
int32 vs int64(long)

Thoughts? @toddbaert @thomaspoignant

@thomaspoignant
Copy link
Member Author

thomaspoignant commented Jul 24, 2022

float vs float64(double) vs float128(decimal)
int32 vs int64(long)

It is a bit too detailed from my point of view.
Typescript does not have a diff between int and float, in java float is not standard but they are using double, etc …
I would avoid being precise here.

@toddbaert
Copy link
Member

toddbaert commented Jul 24, 2022

It is a bit too detailed from my point of view.
Typescript does not have a diff between int and float, in java flot is not standard but they are using double, etc …
I would avoid being precise here.|

Agreed. We also mention this, in the types.md doc:

A numeric value of unspecified type or size. Implementation languages may further differentiate between integers, floating point numbers, and other specific numeric types and provide functionality as idioms dictate.

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.

specification.json Outdated Show resolved Hide resolved
@toddbaert toddbaert self-requested a review July 24, 2022 12:43
Copy link
Member

@toddbaert toddbaert left a 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 <thomas.poignant@gofeatureflag.org>
@thomaspoignant
Copy link
Member Author

BTW the ci is failing on this link https://www.w3.org/TR/qaframe-spec/ that is working fine from where am I.

specification.json Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member

BTW the ci is failing on this link https://www.w3.org/TR/qaframe-spec/ that is working fine from where am I.

Re-ran it, seems to work now. Probably the site was slow for a time.

@toddbaert toddbaert requested a review from moredip July 25, 2022 12:06
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Copy link
Member

@beeme1mr beeme1mr left a 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! 😄

@toddbaert
Copy link
Member

Anyone want to add a final approval for this? Seems like we have consensus.

Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
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.

None yet

5 participants