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

Add conversions for US customary volumes to metric (liters and milliliters) #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sferik
Copy link
Contributor

@sferik sferik commented Mar 28, 2016

Unit conversion numbers taken from Wikipedia.

In addition to adding this new functionality (and specs), I also cleaned up a bunch of RSpec warnings about error expectations without a specified error class.

@sferik sferik force-pushed the us_customary_volumes_to_metric branch from e0004e7 to e000f76 Compare March 28, 2016 18:28
Using the `raise_error` matcher without providing a specific
error or message risks false positives, since `raise_error` will
match when Ruby raises a `NoMethodError`, `NameError` or
`ArgumentError`, potentially allowing the expectation to pass
without even executing the method you are intending to call.
@@ -8,6 +8,8 @@
unit.convert_to(:'fl oz') { |value| value * 128.0 }
unit.convert_to(:tbsp) { |value| value * 256.0 }
unit.convert_to(:tsp) { |value| value * 768.0 }
unit.convert_to(:ml) { |value| value * 3785.411784 }
unit.convert_to(:l) { |value| value * 3.785411784 }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work in conjunction with this require approach. Some users do not want to load all unit definitions, but simply the ones they need. If someone loads just us_customary or us_customary/volume definitions, then these definitions won't work.

Perhaps these belong in a separate require, but let me think on it. It might just make sense to expose an interface for adding custom conversions within your own project as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of your approach was to allow people to selectively require source units (i.e. what they were converting from) but, once that was required, to allow conversion to any destination units of the same type (e.g. volume ➡️ volume).

I think it’s quite common to want to convert units across the US/metric boundary and I’d hope this library would allow that functionality, even if that means offering a less granular require options, the value of which is less obvious, at least to me.

@MyklClason
Copy link

MyklClason commented Dec 2, 2016

I've found you can write an initializer that will override the existing definitions. For example:

# config/initializers/ruby_measurement/volume.rb
...
Measurement.define(:'c.') do |unit|
  unit.alias :c, :cup, :cups
  unit.convert_to(:gal) { |value| value / 16.0 }
  unit.convert_to(:qt) { |value| value / 4.0 }
  unit.convert_to(:pt) { |value| value / 2.0 }
  unit.convert_to(:'fl oz') { |value| value * 8.0 }
  unit.convert_to(:tbsp) { |value| value * 16.0 }
  unit.convert_to(:tsp) { |value| value * 48.0 }
  unit.convert_to(:ml) { |value| value * 236.5882365 }
  unit.convert_to(:l) { |value| value * 0.2365882365 }
end
...

Hardly ideal, but might be useful. Perhaps make it easier to monkey patch existing measurements as well. Though perhaps I'm missing something in that regard. Perhaps include an extend method or something that could be used for adding additional aliases and conversions. So that one could simply use:

Measurement.extend(:'c.') do |unit|
  unit.alias :cuppa, :cuppas # Couldn't think of a better example here
  unit.convert_to(:ml) { |value| value * 236.5882365 }
  unit.convert_to(:l) { |value| value * 0.2365882365 }
end

@mhuggins
Copy link
Owner

mhuggins commented Dec 2, 2016

@MyklClason That's something I always intended to add, but never got around to. I haven't needed this library in awhile, but I'm open to accepting changes for it!

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

Successfully merging this pull request may close these issues.

3 participants