Unable to parse string

[Zurga]

I am experiencing the same issue as #202 .

My current setup is the following: Nixos: 23.11 Dependencies: appsignal 2.7.3 appsignal_phoenix 2.3.2 appsignal_plug 2.0.13 artificery 0.4.3 bamboo 2.1.0 bamboo_phoenix 1.0.0 bamboo_smtp 4.0.1 bcrypt_elixir 2.3.1 browser 0.4.4 bunt 0.2.1 burnex 3.1.0 castore 1.0.4 cc_precompiler 0.1.9 certifi 2.12.0 cldr_utils 2.24.2 comeonin 5.4.0 cowboy 2.10.0 cowboy_telemetry 0.3.1 cowlib 2.12.1 credo 1.7.1 dart_sass 0.7.0 db_connection 2.6.0 decimal 1.9.0 decorator 1.4.0 distillery 2.1.1 dns 2.2.0 earmark 1.4.46 ecto 3.10.3 ecto_commons 0.3.3 ecto_sql 3.10.2 edeliver 1.9.2 elixir_make 0.7.7 esbuild 0.8.1 ex_cldr 2.37.5 ex_cldr_territories 2.2.2 ex_phone_number 0.4.3 exexif 0.0.5 expo 0.4.1 file_system 0.2.10 finch 0.16.0 fuzzy_compare 1.0.0 gen_smtp 1.1.1 gettext 0.23.1 glob_ex 0.1.6 hackney 1.20.1 hpax 0.1.2 httpoison 1.8.2 hut 1.3.0 idna 6.1.1 image 0.39.3 inflex 2.1.0 jason 1.4.1 luhn 0.3.3 metrics 1.0.1 mimerl 1.2.0 mint 1.5.1 miss 0.1.5 mneme 0.4.3 mogrify 0.9.3 mollie 0.7.2 money 1.12.3 msgpax 2.4.0 nimble_options 1.0.2 nimble_pool 1.0.0 owl 0.7.0 parse_trans 3.4.1 phoenix 1.7.10 phoenix_ecto 4.4.3 phoenix_html 3.3.3 phoenix_live_reload 1.4.1 phoenix_pubsub 2.1.3 phoenix_template 1.0.3 phoenix_view 2.0.3 plug 1.15.2 plug_cowboy 2.6.1 plug_crypto 2.0.0 postgrex 0.17.3 ranch 1.8.0 rewrite 0.7.1 socket 0.3.13 sourceror 0.12.3 ssl_verify_fun 1.1.7 surface 0.11.1 surface_font_awesome 0.4.2 surface_markdown 0.6.1 surface_utils 0.1.0 sweet_xml 0.7.4 telemetry 0.4.3 telemetry_metrics 0.6.1 telemetry_poller 0.5.1 tzdata 1.1.1 unicode_util_compat 0.7.0 vix 0.26.0 websock 0.5.3 websock_adapter 0.5.5

[samm81]

I stumbled into this issue as well today.

The issue is that running the example in the docs for Money.parse/1 results in an error:

iex> Money.parse("$1,234.56", :USD)
:error # should be `{:ok, %Money{amount: 123456, currency: :USD}}` according to the docs

the problem for me (and I presume for the others who asked this question) is that we have Decimal installed, but we have a version before 2.0.0. the Decimal.parse/1 signature changed in 2.0.0 to @spec parse(binary()) :: {t(), binary()} | :error (which is what Money.parse/1 expects) from parse(String.t()) :: {:ok, t()} | :error.

so our installs of Decimal are returning {:ok, t()} which is failing to match the case case on money/lib/money.ex#L146, and so the rescue is returning :error.

and Decimal.parse/1 was only added in 1.2.0 ! so for someone who had a version of Decimal before 1.2.0 they would be getting an :error because Decimal.parse/1 doesn't even exist :)

in my case, Decimal isn't even a direct dependency of mine, it's a dependency of a dependency. despite unlocking I can't upgrade Decimal at the moment. as a workaround I can do my own parsing.

as for a long-term fix, there's maybe an option to dynamically fetch the Decimal library version at runtime and check that it's at least 1.2.0, and then match against {:ok, t()} if it's ~> 1.2.0 and match against {t(), binary()} if it's ~> 2.0.0. and throw an error if it's < 1.2.0 ?

and that also wouldn't account for any breaking api changes if Decimal were to update to ~> 3.0.0.

the problem, it seems, is that Decimal is an "unofficial dependency", it's expecting the Decimal api of ~> 2.0.0 but since it's not in the mix.exs deps, mix deps can't do dependency checking. I'm not actually sure what a good solution would be here - obviously other libraries also employ this pattern (eg ecto and postgrex) but I'm not sure how they handle versioning.

[gabrielperales]

@samm81 we can also add a pattern-matching by {:ok, float} -> parse(float, currency, []) here without checking the version of the library. Would it work?

[Zurga]

Checking the version is probably the best option in terms of completeness, but I wonder if there are many projects that depend on Decimal < 1.2.0.

If checking the version of Decimal at runtime gives a performance hit, I would say the extra pattern match (with a comment explaining why it is there) is the way to go.

[samm81]

ah yes @Zurga posted his dependencies and decimal is 1.9.0

[Zurga]

@samm81 your solution would have helped in my situation too :smile:

[samm81]

@gabrielperales @Zurga the pattern match would work but would fail when Decimal < 1.2.0 or if the api breaks in some hypothetical future version of Decimal

maybe the quick fix is to add the pattern match and update the readme?

[gabrielperales]

I gave it a try :) @samm81 @Zurga https://github.com/elixirmoney/money/pull/227

[Zurga]

Good work! Blend looks like a nice tool, I had not heard about it before this PR.