search

Shopify / measured

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

Implement `Measured::Temperature` #59

Open kmcphillips opened 7 years ago

kmcphillips commented 7 years ago

Our unit system conversion assumes all units share a 0 and there is a multiplicative factor between them.

This falls apart for temperature.

To assure we are making valid assumptions and have a relatively generic conversion table system, we should implement temperature conversion. Between F/C/K.

We can not include it by default. But put it into the README and show how to include it.

We can use ActiveSupport::Testing::Isolation to test it if needed without requiring it globally: http://api.rubyonrails.org/classes/ActiveSupport/Testing/Isolation.html

jesusmgg commented 3 years ago

Why can't this be included by default if implemented?

I'm working on my own implementation and have something working for now: https://github.com/jesusmgg/measured/tree/temperature-support. I've added support for base offsets in unit definitions, and take this into account when doing conversions. So far, it works with every unit I throw at it, no matter the offset direction or conversion factor. I still need to make the cache support this, and add unit tests.

I know this is an ancient ticket so feel free to tell me not to reply here anymore.

kmcphillips commented 3 years ago

Oh I'll gladly review and merge this!

What I mean by "include it by default" is include it here: https://github.com/Shopify/measured/blob/504a858c88b74523887abf1e77782223496c638c/lib/measured.rb#L4-L6

There is significant overhead to each unit system. Loading the conversion table takes time and memory. So including every new unit system by default would introduce nontrivial performance regressions to every app which uses this gem and upgrades. And that's just not acceptable.

Admittedly probably none of the included unit systems should be included by default, because if they're unused that's overhead, but removing them now is a breaking change. So that's not top of mind.

The docs can be cleaned up to be clearer, but basically to do temperature conversion you'll put this in your gemfile:

gem 'measured', require: ['measured/base', 'measured/temperature']
jesusmgg commented 3 years ago

That sounds great and makes sense, I'll keep working on it and clean up my code and then submit it then.

kmcphillips commented 3 years ago

Ok well let me know if you have questions or I can help.