Shopify / measured

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

Add optional rounding to the string representation #98

Closed javierhonduco closed 6 years ago

javierhonduco commented 6 years ago

Hiii,

In #96 @alexgomez54 and I talked about a use case we had of stringifying a Measurable object in which we wanted to be able to round the number of displayed decimals.

This PR adds that functionality in the #to_s method, with an optional keyword argument round, which is nil by default. In case nil is received, it does the old logic. Otherwise, it rounds the float before building the string for us.

Why this approach?

That being said, I feel that maybe having this optional keyword argument in #to_s, such a common method, may feel weird. Was wondering if you think it would make more sense to have another method like#to_s_with_rounding or something on that. Happy to change it!! 😃

javierhonduco commented 6 years ago

Personally I'm not a fan of changing the interface to such a method

Totally agree 😄 . Just wanted to hear other people's opinions here.

javierhonduco commented 6 years ago

@kmcphillips Makes sense 👍.

#round sounds like it's going to return the rounded float, what do you think about about something in the lines of #to_s_with_rounding?

thegedge commented 6 years ago

#round sounds like it's going to return the rounded float

Which would make the most sense, but I'll call out my comment from the linked issue as to why I think it's a bad idea:

My only concern here is that you have to be very aware of the underlying unit for round to be significant. For example, think about let's say a = 1g and b = 0.001kg . Now we have a == b but a.round(2) != b.round(2) which feels like kind of a weird property to introduce.

#to_s_with_rounding Is a little verbose though. I'm still inclined to push this logic to a presentation layer 😉

Another idea: configurable formatter for Measurable.

javierhonduco commented 6 years ago

Which would make the most sense, but I'll call out my comment from the linked issue as to why I think it's a bad idea:

I completely agree with the thing you mentioned a while ago, I didn't spot it back then 😄

to_s_with_rounding Is a little verbose though.

Agree! Not the most beautiful name 😢

I'm still inclined to push this logic to a presentation layer 😉

No strong opinions on my side, we could move this to measured-rails 😄 , I don't know how would be the best way of doing so, but we'll figure it out :)

Another idea: configurable formatter for Measurables.

That's interesting! Something in the lines of: Measurable::format("%.2f %s", <Measure_object>)>? (I always get the floating part wrong, though!!)

thegedge commented 6 years ago

That's interesting! Something in the lines of: Measurable::format("%.2f %s", )>? (I always get the floating part wrong, though!!)

That would probably make sense as Measurable#format too, with a less verbose name that to-s_with_rounding and some flexibility in representation :)

You could even use Kernel#sprintf (a.k.a., String#%) so that people can reference value / unit:

irb> "%.2<foo>f" % {foo: 0.123789123789123}
=> "0.12"
irb> foo.format("%.2<value>f---%<unit>")
10.34---L
javierhonduco commented 6 years ago

Oops, yeah I meant #format but I used :: instead 🙄 .

TIL about being able to use keywords in String#%, super cool!!

I really like this approach!! 😄 Should we go for it here or in -rails?

thegedge commented 6 years ago

I'd be totally okay with adding it in measured. Maybe one of the other reviewers can 👍 that before you move forward, so we don't send you down a path that will result in ❌ instead of ✅

alexgomez54 commented 6 years ago

:+1: to the idea of adding a class method for format. This is 100% a presentation concern in the use cases I'm thinking of. Extracting this to a formatter makes for a good home for a few other items on my wish list:

I spoke to @javierhonduco IRL and this is the use case I had in mind: An array of 3 measured objects that I need to format as 10 x 11 x 12 cm. It would be great to map the proposed formatter through it.