search

Shopify / measured

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

Have Measured.build return a UnitSystem subclass #71

Closed thegedge closed 7 years ago

thegedge commented 7 years ago

One of the last big changes to make before Measured 2.0, Measured.build will now return a dynamically-defined subclass of UnitSystem instead of Measurable. Adds a little complexity in some places, but I think the semantics of building a unit system make more sense than a measurable subclass that has class method pointing to the unit system.

Recommend adding w=1 to the query string. Additional comments in line.

Will switch to merging into master after https://github.com/Shopify/measured/pull/70 merged.

thegedge commented 7 years ago

^ Just rebasing on top of the newly rebased store-unit-instead-of-name branch

thegedge commented 7 years ago

I just realized a way I could do this by giving the unit instances a unit system. I'd much rather do this, so I'll add some new commits with the changes necessary to do just that.

[EDIT] Actually, I'm going to do that in a separate PR.

thegedge commented 7 years ago

Rebased. Ready for review again.

@nicolaslupien only the last two commits are new since your review, so feel free to just look at those.

ghost commented 7 years ago

:shipit:

thegedge commented 7 years ago

After chatting with @kmcphillips in :slack: space, we've decided to drop this PR because of https://github.com/Shopify/measured/pull/71#discussion_r96704848.

I'm in favour of the semantics this PR introduces (specifically, having Measured.build return a UnitSystem), but right now it's not offering enough real benefits to make the switch. We can return to this in a future release.