search

Shopify / measured

Encapsulate measurements and their units in Ruby and Ruby on Rails.
MIT License
337 stars 28 forks source link

Do not allow arithmetic with non-Measured types #46

Closed thegedge closed 7 years ago

thegedge commented 7 years ago

Without a unit, it's really hard to understand intent and could lead to subtle bugs / surprises. For example, what does Measured::Length.new(1, :cm) + 3 mean? Was the intent to add 3cm? This is a case where explicitness is a must.

Although we could argue that identity elements – 1 for multiplication / division, 0 for addition and subtraction (when on the RHS) – would often make sense, they don't always. For example, 0°C + 10°F is 42°F, not 10°F.

thegedge commented 7 years ago

Actually, after thinking about it, a unitless multiplication / division makes sense since 1m * 1m would be a squared metre. We need units for addition, subtraction, and comparison.

kmcphillips commented 7 years ago

Yes, exactly!

thegedge commented 7 years ago

So we have one issue here: Ruby is terrible for implementing this exact behaviour. When you have a <op> b you basically get bb, aa = a.coerce(b); aa <op_without_coerce> b). In other words, to support Numerics on the left for multiplication, we also have to allow addition.

thegedge commented 7 years ago

I think the case for preventing 3 + Measured::Length(0, :cm) and Measured::Length(0, :cm) + 3 is stronger than supporting multiplication. My vote is to implement a multiply or scale method that does multiplication.

jonathankwok commented 7 years ago

Could we raise an error or something explaining the existence of multiply if they use *, if we pursue that route?

thegedge commented 7 years ago

Yep, we can include that in the TypeError exception message. I'll probably lean towards scale instead of multiply given that you could also multiply two Measurables together to get a squared measurable. We could also do inner_product which perfectly describes what we'd like to do, but I fear that's a little too mathematical and may be a confusing name.

jonathankwok commented 7 years ago

Oh, scale is pretty great, easy to understand and better reflects what we're trying to do anyway. Agree on the inner_product being a little too mathy, especially if used in the context of packages/products and whatnot.

thegedge commented 7 years ago

Closed in https://github.com/Shopify/measured/pull/49. Will add a #scale method though.