Skip to content

Add Decimal.from_any/1 function to handle int, float, and binary input#116

Merged
ericmj merged 3 commits intoericmj:masterfrom
eqmvii:add-new-from-any-function-to-handle-int-float-and-binary-input
May 23, 2019
Merged

Add Decimal.from_any/1 function to handle int, float, and binary input#116
ericmj merged 3 commits intoericmj:masterfrom
eqmvii:add-new-from-any-function-to-handle-int-float-and-binary-input

Conversation

@eqmvii
Copy link
Copy Markdown
Contributor

@eqmvii eqmvii commented May 6, 2019

We've had issues with warnings from passing a float to Decimal.new/1 in some of the 1,000+ calls in our production Elixir application, since sometimes we want to create a Decimal without knowing the type we're passing in ahead of time.

We created this new function to handle that scenario in our app, then realized it might be better as part of the public interface of this library. Since float conversion is not exact, its module doc encourages people to use Decimal.new/1 or Decimal.from_float/1 instead whenever possible.

We also noticed that Ecto implemented a decimal_new/1 to workaround these same warnings, so if this is added to Decimal, it could be used instead of that work-around in Ecto:

https://github.com/elixir-ecto/ecto/blob/6bc632e90f0639e4643c77e599b357dac2a9acb4/lib/ecto/changeset.ex#L2047

And it would more explicitly allow for the functionality requested in this earlier PR: #109

Thanks for your consideration!

@ericmj ericmj merged commit af14a72 into ericmj:master May 23, 2019
"""
@spec from_any(number | binary) :: t
def from_any(float) when is_float(float), do: from_float(float)
def from_any(value), do: new(value)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR!

because we use Decimal.new under the hood, this will work too:

iex> Decimal.from_any(Decimal.new(42))
#Decimal<42>

Could you send another patch with an update to docs and specs to mention we accept decimals here too? Theres a decimal :: t | integer | String.t type that you can use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this will be a big help for us. And I'd be happy to submit that update as well, good call!

@wojtekmach
Copy link
Copy Markdown
Collaborator

wojtekmach commented Jun 8, 2019

I'd like to revisit the name of this function. The problem is it doesn't really work with any type, just a small subset of types, so by calling it from_any it might be a bit confusing.

Here's a couple ideas:

  • unsafe_new
  • imprecise_new
  • cast
  • coerce
  • from_number_or_string

I personally like the 1st option the most. What do you think?

@eqmvii
Copy link
Copy Markdown
Contributor Author

eqmvii commented Jun 9, 2019

Happy to revisit the name! I really like cast - it concisely implies the more significant conversion without over-promising on functionality the way from_any did. I also checked and cast wasn't used elsewhere in the library, so there's no risk of confusion there. I'll submit a pull request with that change.

Here's more detail on why I prefer it to the others:

  • unsafe_new and imprecise_new: I don't think the extra warning is needed, as it's no less safe or precise than the existing from_float that it calls, and there is strong warning in the documentation.

  • coerce: probably my 2nd favorite, and also conveys what's happening clearly and concisely.

  • from_number_or_string: good and self documenting, but since cast feels as good and is more concise, I prefer it.

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.

3 participants