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

Add support for double feature flags #2029

Merged
merged 4 commits into from
Jul 15, 2021

Conversation

ThePumpingLemma
Copy link
Collaborator

While here, make some minor cleanups + reorganization for boolean
feature flags

While here, make some minor cleanups + reorganization for boolean
feature flags
Copy link
Collaborator

@r3mariano r3mariano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wary of potential floating-point precision losses with Double and the potential for confusion there, e.g. featureFlags.getDouble(...) != 0.3 despite it being set on the feature flag UI. (I see that the LD SDK uses a double too, and that uses Gson under the hood. See for instance google/gson#1127)

@escardin
Copy link
Contributor

escardin commented Jul 7, 2021

I'm wary of potential floating-point precision losses with Double and the potential for confusion there, e.g. featureFlags.getDouble(...) != 0.3 despite it being set on the feature flag UI. (I see that the LD SDK uses a double too, and that uses Gson under the hood. See for instance google/gson#1127)

Sure, but why do we care? Every flag I've looked at in older systems that does this is using it for something like a percentage rollout. The difference between 0.3 and 0.299999999999999988897769753748434595763683319091796875 for all practical purposes are the same. We also offer floats, and that's fine too.

I'm much more concerned about the double->long conversion, as I know people store epoch timestamps and expect them to parse, and that's too big to reliably represent

IMO if you need that level of precision in your decimal number... don't use a double, and you should already know that, because you learned not to use floating point for decimals/money already.

@r3mariano
Copy link
Collaborator

Sure, but why do we care? Every flag I've looked at in older systems that does this is using it for something like a percentage rollout. The difference between 0.3 and 0.299999999999999988897769753748434595763683319091796875 for all practical purposes are the same. We also offer floats, and that's fine too.

This isn't a percentage rollout, it's a decimal number flag value. I get that they are practically the same, but I'm not sure if a computer will always think so too, hence my previous example. In any case, happy to try it out.

IMO if you need that level of precision in your decimal number... don't use a double, and you should already know that, because you learned not to use floating point for decimals/money already.

Misk's a pretty "guard-rails"-heavy framework IMO. Even if parts of it do right now, we shouldn't rely on "you should know that".

@ThePumpingLemma
Copy link
Collaborator Author

@r3mariano are you okay with me merging this?

@chris-ryan-square chris-ryan-square merged commit 4b10969 into cashapp:master Jul 15, 2021
mightyguava pushed a commit to cashapp/wisp that referenced this pull request Jul 12, 2022
While here, make some minor cleanups + reorganization for boolean
feature flags

Co-authored-by: Ricardo Mariano <rmariano@squareup.com>
Co-authored-by: Chris Ryan <74329397+chris-ryan-square@users.noreply.github.com>
rainecp pushed a commit that referenced this pull request Jul 24, 2023
While here, make some minor cleanups + reorganization for boolean
feature flags

Co-authored-by: Ricardo Mariano <rmariano@squareup.com>
Co-authored-by: Chris Ryan <74329397+chris-ryan-square@users.noreply.github.com>
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.

4 participants