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

Another Ruby 2.6 BigDecimal compatibility issue #34612

Conversation

@utilum
Copy link
Contributor

@utilum utilum commented Dec 4, 2018

This patch modifies XmlMini::Parsing["decimal"] to handle a string that contains an invalid number. Since ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144 this case raises an ArgumentError. String.to_f returns 0.0 if there is not a valid number at the start of the argument, so current behavior is conserved.

See https://travis-ci.org/rails/rails/jobs/463180341#L6264

Related: #34600, #34601

This patch modifies XmlMini::Parsing["decimal"] to handle a string that
contains an invalid number. Since [ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144](ruby/ruby@a0e438c#diff-6b866d482baf2bdfd8433893fb1f6d36R144) this case raises an `ArgumentError`. `String.to_f` returns 0.0 if there is not a valid number at the start of the argument, so current behavior is conserved.

See https://travis-ci.org/rails/rails/jobs/463180341#L6264

Related: #34600, #34601
@@ -71,7 +71,7 @@ def content_type
begin
BigDecimal(number)
rescue ArgumentError
BigDecimal("0")
BigDecimal(number.to_f.to_s)

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 4, 2018
Member

Why not BigDecimal(0)?

This comment has been minimized.

@utilum

utilum Dec 4, 2018
Author Contributor

BigDecimal("0") returns the same value as BigDecimal(0): 0.0. If this is what we want, we can tweak the test.

I understand from the test that we expect BigDecimal("123,003") to return 123.0, and to do that we need to pass it a cleaned-up string.

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 4, 2018
Member

Ok. I got it. Before BigDecimal("123,003") did not raise and now it does.

This comment has been minimized.

@utilum

utilum Dec 4, 2018
Author Contributor

Exactly.
Thank you.

@rafaelfranca rafaelfranca merged commit 1decfed into rails:master Dec 4, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Dec 4, 2018
…tring_argument

Another Ruby 2.6 BigDecimal compatibility issue
@utilum utilum deleted the utilum:bigdecimal_raises_on_comma_in_string_argument branch Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants