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

Make Money.to_string! actually raise if there is an error #103

Closed
wants to merge 2 commits into from
Closed

Make Money.to_string! actually raise if there is an error #103

wants to merge 2 commits into from

Conversation

rodrigues
Copy link

Hi @kipcole9, I was reading the code and found that to_string! doesn't fail in case to_string returns an error tuple. Instead, it just returns the error tuple.

Wrote a test that confirmed that, and made to_string! raise Cldr exceptions.

However, I couldn't write a test that reaches the more generic clause, as I don't know if it can return an error different than {:error, {cldr_error_type, message}}.

@kipcole9
Copy link
Owner

kipcole9 commented Jun 2, 2019

@rodrigues Thanks very much for the report and PR.

I've manually merged the code and tests with just a slight modification since I can confirm that the error return for to_string/2 is {:error, {atom, String.t}}.

I have one dialyzer issue on the spec that is puzzling (overlapping contract) but the I've pushed this commit to master branch of the repo.

I'll publish a new release to hex when I resolve the dialyzer issue.

@kipcole9
Copy link
Owner

kipcole9 commented Jun 2, 2019

Fixed the dialyzer error and published version 3.4.3 to hex.

Many thanks again.

@kipcole9 kipcole9 closed this Jun 2, 2019
@rodrigues rodrigues deleted the to_string_error branch June 2, 2019 16:06
@rodrigues
Copy link
Author

rodrigues commented Jun 2, 2019

Thanks @kipcole9 !

FYI, I discovered this while investigating this other issue: https://elixirforum.com/t/excruciatingly-slow-performance-of-ex-money-money-to-string/22876

@kipcole9
Copy link
Owner

kipcole9 commented Jun 2, 2019

Fixed the performance issue in ex_cldr with version 2.7.1

Really appreciate the assistance, thanks.

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.

2 participants