Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Measured::Temperature #59

Open
kmcphillips opened this issue Jan 10, 2017 · 4 comments
Open

Implement Measured::Temperature #59

kmcphillips opened this issue Jan 10, 2017 · 4 comments

Comments

@kmcphillips
Copy link
Contributor

kmcphillips commented Jan 10, 2017

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

@kmcphillips kmcphillips added this to the Version 2.0 milestone Jan 10, 2017
@thegedge thegedge removed this from the Version 2.0 milestone Apr 5, 2017
@jesusmgg
Copy link

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
Copy link
Contributor Author

kmcphillips commented Nov 27, 2020

Oh I'll gladly review and merge this!

What I mean by "include it by default" is include it here:

require "measured/units/length"
require "measured/units/weight"
require "measured/units/volume"

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
Copy link

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

@kmcphillips
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants