search

Shopify / measured-rails

Rails adapter for the measured gem. Encapsulate measurements and their units in Ruby and Rails.
MIT License
92 stars 13 forks source link

Fix check for BigDecimal precision #14

Closed garethson closed 9 years ago

garethson commented 9 years ago

Our validation for column size had a small bug in that the check for precision could fail:

[1] pry(#<Thing>)> BigDecimal.new('2000000000000').split
=> [1, "2", 10, 13]

We then to "2".length which was < precision and our check failed us.

@trishume @kmcphillips

trishume commented 9 years ago

This isn't correct at all. This doesn't count the number of significant digits, it counts something totally different. BigDecimal#to_s doesn't output what you expect:

[16] pry(main)> BigDecimal(5).to_s
=> "0.5E1"
trishume commented 9 years ago

The check that is there already (not the one in this PR) looks correct to me. The only thing I see is that it should also have a check to see if the exponent fits in the decimal column as well.

trishume commented 9 years ago

Oh turns out I misunderstood and thought MYSQL decimal columns were like BigDecimal's digits+exponent but no they are fixed-point which explains the issue.

https://dev.mysql.com/doc/refman/5.1/en/precision-math-decimal-characteristics.html

What this needs is a check on the number of digits to the left of the decimal place.

garethson commented 9 years ago

@trishume

What this needs is a check on the number of digits to the left of the decimal place.

That's our intention here, I"m adding more tests, but I'm missing a .to_i, which uses an int or a bigint if it needs to.

garethson commented 9 years ago

if rounded_to_scale_value.to_i.to_s.length > (precision - scale)

trishume commented 9 years ago

@garethson cool that looks right to me. Only change is as I mentioned in a previous comment I think the rounded_to_scale step should be removed, it's risky and I don't think it adds any value.