Skip to content

Have different methods for float and int#116

Merged
justinabrahms merged 7 commits into
open-feature:mainfrom
thomaspoignant:evaluation-proposition
Jul 26, 2022
Merged

Have different methods for float and int#116
justinabrahms merged 7 commits into
open-feature:mainfrom
thomaspoignant:evaluation-proposition

Conversation

@thomaspoignant

@thomaspoignant thomaspoignant commented Jul 23, 2022

Copy link
Copy Markdown
Member

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

@thomaspoignant
spec flag evaluation float int types
c6a4f3a 
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
@thomaspoignant
Rebuild json file
9512910 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
toddbaert
toddbaert reviewed Jul 24, 2022
View reviewed changes
Comment thread specification/flag-evaluation.md
toddbaert
toddbaert reviewed Jul 24, 2022
View reviewed changes
Comment thread specification/flag-evaluation.md
toddbaert
toddbaert requested changes Jul 24, 2022
View reviewed changes

@toddbaert toddbaert left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

toddbaert commented Jul 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

@toddbaert toddbaert requested a review from InTheCloudDan July 24, 2022 04:46
@justinabrahms

justinabrahms commented Jul 24, 2022

Copy link
Copy Markdown
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. :)

@thomaspoignant
Fix review comments
057a80f 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant thomaspoignant force-pushed the evaluation-proposition branch from d17dba5 to 057a80f Compare July 24, 2022 08:57
@thomaspoignant
linting issue
2869a85 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant thomaspoignant requested a review from toddbaert July 24, 2022 08:59
@benjiro

benjiro commented Jul 24, 2022

Copy link
Copy Markdown
Member

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

thomaspoignant commented Jul 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

toddbaert commented Jul 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

toddbaert
toddbaert reviewed Jul 24, 2022
View reviewed changes
Comment thread specification.json Outdated
@toddbaert toddbaert self-requested a review July 24, 2022 12:43
toddbaert
toddbaert approved these changes Jul 24, 2022
View reviewed changes

@toddbaert toddbaert left a comment

Copy link
Copy Markdown
Member

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

@thomaspoignant
Specify all the types
f1ca6b9 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant

thomaspoignant commented Jul 25, 2022

Copy link
Copy Markdown
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.

@thomaspoignant thomaspoignant requested a review from toddbaert July 25, 2022 09:35
toddbaert
toddbaert reviewed Jul 25, 2022
View reviewed changes
Comment thread specification.json Outdated
@toddbaert

toddbaert commented Jul 25, 2022

Copy link
Copy Markdown
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
@thomaspoignant
fix after Todd review
4ee9082 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
beeme1mr
beeme1mr approved these changes Jul 25, 2022
View reviewed changes

@beeme1mr beeme1mr left a comment

Copy link
Copy Markdown
Member

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

toddbaert commented Jul 26, 2022

Copy link
Copy Markdown
Member

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

justinabrahms
justinabrahms reviewed Jul 26, 2022
View reviewed changes
Comment thread specification/flag-evaluation.md Outdated
@thomaspoignant
mention only numbers
505c263 
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
@thomaspoignant thomaspoignant force-pushed the evaluation-proposition branch from 26c804f to 505c263 Compare July 26, 2022 16:05
@justinabrahms justinabrahms merged commit 7d4648c into open-feature:main Jul 26, 2022
@thomaspoignant thomaspoignant mentioned this pull request Jul 26, 2022
Closed
@thomaspoignant thomaspoignant deleted the evaluation-proposition branch July 26, 2022 16:09
@benjiro benjiro mentioned this pull request Aug 4, 2022
Merged
This was referenced Aug 5, 2022
Closed
Closed
Closed
Closed
Closed
@dd-oleksii dd-oleksii mentioned this pull request May 26, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toddbaert toddbaert toddbaert approved these changes

@justinabrahms justinabrahms justinabrahms approved these changes

@beeme1mr beeme1mr beeme1mr approved these changes

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

@agentgonzo agentgonzo Awaiting requested review from agentgonzo

@benjiro benjiro Awaiting requested review from benjiro

@InTheCloudDan InTheCloudDan Awaiting requested review from InTheCloudDan

@moredip moredip Awaiting requested review from moredip

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thomaspoignant @toddbaert @justinabrahms @benjiro @beeme1mr