-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: master
Are you sure you want to change the base?
Conversation
e0004e7
to
e000f76
Compare
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I've found you can write an initializer that will override the existing definitions. For example:
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
|
@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! |
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.