search

Shopify / measured

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

Don't allow Numeric usage in operations #49

Closed thegedge closed 7 years ago

thegedge commented 7 years ago

Problem

As mentioned in https://github.com/Shopify/measured/issues/46, adding/subtracting Numeric values to/from a Measurable is confusing, since no unit is specified and we'd rather not assume it's in the same unit as the Measurable. We'd like to allow scaling the Measurable by a scalar, but can't do so without allowing addition too.

Solution

Given we want to avoid confusion, we'll remove the ability to coerce Numeric values into Measurables, and require someone to manually do that conversion. This may lead to more verbose code, but hopefully things like https://github.com/Shopify/measured/issues/39 will reduce the verboseness.

In a follow up PR I'll also add a scale method to allow multiplying a Measurable by a scalar.

@kmcphillips @jonathankwok

thegedge commented 7 years ago

Ah yes, thanks for always catching the README (I've gotten so used to not having a README...).

Converts units only as needed for equality comparison: is fine, since we're allowed to compare two measured values (of the same class).

I also added a test to verify that arithmetic operations favour the unit of the measured value on the left-hand side of the operation (mentioned in the README). That being said, I'd rather not enforce this because it should be irrelevant to the client. The fewer constraints we add the more wiggle room we have to optimize the implementations.

thegedge commented 7 years ago

Also, after learning that we can't do multiplication by a scalar on the left, I'm thinking the use of Numeric is not nearly as useful anymore. I may drop that in a follow-up PR.

thegedge commented 7 years ago

As a team we discussed this and came to a consensus that dropping comparison to 0 / 0.0 (but we still allow comparison to a Measured::Thing(0, :some_unit)) is a good thing. For some unit systems comparison to 0 makes sense (e.g., lengths), but for others it doesn't (e.g., temperature).

We definitely understand that some of the changes in this PR, especially the comparison to 0, could make certain things more difficult to do, but we'll offer alternatives to do certain operations. All of this should be a good thing, as clients of this library will now be more explicit about the context of values, which should ensure correctness.