Shopify / measured

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

Measured version 0.0.1 #1

Closed kmcphillips closed 9 years ago

kmcphillips commented 9 years ago

@Shopify/shipping @maartenvg @celsodantas @boourns @jnormore

What is this?

This is version 0.0.1 of a library I have written to encapsulate measurements, weight and dimensions to start with, in a single object.

Currently in Shopify we juggle two different properties which really define a single weight or dimension. We do lossy math with UnitSystem aka Quantified meaning our precision is bad. And we have no atomic way of reading/writing these to columns in the DB with ActiveRecord without juggling the fields by ourselves.

All math is done in this gem with BigDecimal. Conversions are extensible.

Features

I wrote a detailed README. Read it. Please:

https://github.com/Shopify/measured/blob/version-0.0.1/README.md

Why build from scratch?

There are two unit management libs I could find of note. They are contrasted in the README.

tl;dr Quantified in ActiveShipping does only float math and pluralizes all units. Its core assumptions are inaccurate. The ruby-units overrides nearly all core types and depends on those internally. Plus it is huge. Neither have ActiveRecord adapters anyway.

Next steps

Writing a measured-rails adapter for ActiveRecord which understands measured objects and how to store them in columns and convert them. Inspiration for this will be taken from the Money gem.

Something roughly like:

class Box < ActiveRecord::Base
  dimension_column :length
end

Which will load/store data in length_amount and length_unit columns, with optionally normalizing to length_amount_mm or whatever.

kmcphillips commented 9 years ago

@qq99 I have added comments inline to the README to explain what is happening when defining units.

qq99 commented 9 years ago

Awesome, thanks @kmcphillips

robfoster commented 9 years ago
rake test
WARN: Unresolved specs during Gem::Specification.reset:
      json (>= 1.7.7, ~> 1.7)
WARN: Clearing out unresolved specs.
robfoster commented 9 years ago

Added a few comment I hope were interesting but nothing blocking. :shipit: Nice work.

kmcphillips commented 9 years ago

Fixed a bunch of comments. Thanks for all the feedback.

I'm going to go forward with how this is structured, despite some rounding anomalies and precision loss. In a subsequent PR I'm going to circle around some ways to reduce precision loss. One of my best ideas is to see if I can leave the value always represented as a Rational if any of the composite factors are rational. Then let the consumer of the unit do their own coercion, rounding, and precision loss.

Alternately I can change the tests to assert a certain level of rounding and just deal with the fact that we can never be completely perfect. We are already keeping more digits of precision than are specified in the standards.

qq99 commented 9 years ago

:+1:

kmcphillips commented 9 years ago

Renamed Dimension to Length to match standards:

screen shot 2015-03-05 at 10 01 31 am

kmcphillips commented 9 years ago

Ok. I really appreciate the feedback. I'm going to merge this today, release the gem, and then it can be iterated on. I've opened some issues to track that. #2 #3 #4

maartenvg commented 9 years ago

Haven't reviewed in great detail (didn't check each and every operation), but overall it looks complete and elegant. :sparkles: for owning this problem area.

:shipit:! While working with it we'll find any edge cases we've missed.

kmcphillips commented 9 years ago

Appreciate it!

qq99 commented 9 years ago

:tada:

garethson commented 9 years ago

:clap:!