Navigation Menu

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

Improve BigDecimal mongoize behavior #4164

Merged
merged 1 commit into from Apr 21, 2016

Conversation

johnnyshields
Copy link
Contributor

Recently I attempted to migrate a number of Float fields to BigDecimal, and I found that the behavior is inconsistent between the two. This PR rectifies that situation. Note that BigDecimal is stored in MongoDB as a string, as there is no native BSON type for it.

  • demongoize should always return a BigDecimal or else nil, never an empty string (this mirrors how Floats and Integers behave).
  • mongoize should always return a String which can be demongoized to a BigDecimal, otherwise nil
  • Support storing BigDecimal values "Infinity" and "-Infinity" in the database.
  • The strings "Infinity" and "-Infinity" now evaluate as numeric? true.
  • BigDecimals now evaluate as numeric? true.
  • Additional tests for BigDecimal covering all edge cases.

Demongoize

Value to Demongoize Current New Behavior Changed?
"0" BigDecimal 0.0 BigDecimal 0.0
"-0" BigDecimal -0.0 BigDecimal -0.0
"1" BigDecimal 1 BigDecimal 1
"1.1" BigDecimal 1.1 BigDecimal 1.1
nil String "" nil Y
false nil nil
true true (TrueClass) nil Y
"" String "" nil Y
"other1" String "other" nil Y
"NaN" BigDecimal NaN BigDecimal NaN
"Infinity" String "Infinity" BigDecimal Infinity Y
"-Infinity" String "-Infinity" BigDecimal -Infinity Y

Mongoize

Value to Mongoize Current New Behavior Changed?
BigDecimal 0 String "0.0" String "0.0"
BigDecimal -0 String "-0.0" String "-0.0"
BigDecimal 1 String "1" String "1"
BigDecimal 1.1 String "1.1" String "1.1"
nil String "" nil Y
false false (FalseClass) nil Y
true String "true" nil Y
"" String "" nil Y
"1" String "1" String "1"
"1.1" String "1.1" String "1.1"
"other1" String "other" nil Y
String "NaN" String "NaN" String "NaN"
String "Infinity" String "Infinity" String "Infinity"
String "-Infinity" String "-Infinity" String "-Infinity"
BigDecimal NaN String "NaN" String "NaN"
BigDecimal Infinity String "Infinity" String "Infinity"
BigDecimal -Infinity String "-Infinity" String "-Infinity"
Float::NAN String "NaN" String "NaN"
Float::INFINITY String "Infinity" String "Infinity"
Float::INFINITY * -1 String "-Infinity" String "-Infinity"

@johnnyshields
Copy link
Contributor Author

One more note here, since numeric? is only used for BigDecimal, it might make sense to remove it from all the classes where it appears and instead make a private method in BigDecimal (numeric?(object)) which evaluates whether the object is numeric. Less monkey patching that way.

@johnnyshields johnnyshields changed the title Improve support for BigDecimal Improve BigDecimal mongoize behavior Sep 29, 2015
@johnnyshields
Copy link
Contributor Author

I've raised MONGOID-4165 to track this.

@@ -22,18 +70,29 @@
end
end

context "when the value is a float" do
context "when the the value is true" do
Copy link
Contributor

Choose a reason for hiding this comment

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

there's an extra 'the' here

@estolfo
Copy link
Contributor

estolfo commented Apr 21, 2016

Hi @johnnyshields
Thanks again for all your work in this PR.
We are ready to merge this into master now. Would you mind rebasing against master so we can double check that the tests pass on travis? Thanks!

@johnnyshields
Copy link
Contributor Author

@estolfo done. Travis CI running now, looks like it should be good.

@estolfo
Copy link
Contributor

estolfo commented Apr 21, 2016

thanks!

Object.new
end

it "returns true" do
Copy link
Contributor

Choose a reason for hiding this comment

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

this should say false

- `demongoize` should always return a BigDecimal or else nil, never an empty string
- `mongoize` should always return a String which can be demongoized to a BigDecimal, otherwise or nil
- The strings `"Infinity"` and `"-Infinity"` now evaluate as numeric true.
- `BigDecimal`s now evaluate as numeric true.
- Additional tests for BigDecimal covering all edge cases.
- Fix typo "the the" in all specs / comments in repo
@johnnyshields
Copy link
Contributor Author

@estolfo I fixed all the notes you raised.

@estolfo
Copy link
Contributor

estolfo commented Apr 21, 2016

thanks!

@estolfo estolfo merged commit b1866c2 into mongodb:master Apr 21, 2016
@johnnyshields johnnyshields deleted the big-decimal branch April 21, 2016 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants