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

Money.Sigil requires existence of the atom for the currency #46

Closed
marcelotto opened this issue Jan 17, 2018 · 3 comments
Closed

Money.Sigil requires existence of the atom for the currency #46

marcelotto opened this issue Jan 17, 2018 · 3 comments

Comments

@marcelotto
Copy link

Since the implementation of Money.Sigil relies on String.to_existing_atom/1 it can't be used without former usage of the atom for the currency:

Interactive Elixir (1.5.1) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> import Money.Sigil
Money.Sigil
iex(2)> ~M[42]EUR
** (ArgumentError) argument error
    :erlang.binary_to_existing_atom("EUR", :utf8)
    (ex_money) lib/money/sigil.ex:34: Money.Sigil.atomize/1
    (ex_money) lib/money/sigil.ex:21: Money.Sigil.sigil_M/2

I've noticed this similar issue, but the fix got removed later.

@kipcole9
Copy link
Owner

Thanks for the bug report. Fixed in master and also published as version 2.0.2 on hex.

@kipcole9
Copy link
Owner

kipcole9 commented Jan 18, 2018

I also researched issue #8 that you referenced. I reintroduced this issue when I repackaged ex_cldr into several smaller dependencies. Reintroducing an old bug is definitely not great.

I haven't found a good way to test for this specific error since I don't know how to ensure a test has a "no modules loaded yet" state to start with. I've added some tests for the negative case (i.e. raises error on invalid currency code in the sigil).

I'll ask the forums help for a way to unload all modules before a test runs - if you have any thoughts they'd be appreciated.

@marcelotto
Copy link
Author

Thank you very much for your quick response and the immediate fix.

Sorry, I don't know how to test this properly.

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

No branches or pull requests

2 participants