search

tmlee / time_difference

The missing Ruby method to print out time difference (duration) in year, month, week, day, hour, minute, and second.
https://rubygems.org/gems/time_difference
MIT License
305 stars 92 forks source link

Add supports ActiveSupport > 5.1 - SI base units) #31

Closed badlamer closed 6 years ago

badlamer commented 7 years ago

In rails/rails#27610 changes duration values

Add ruby 2.3 and 2.4 in tests.

aviflax commented 7 years ago

Do you think this will address ~#31~?

Er, sorry, I meant #32.

aviflax commented 7 years ago

Do you think this might address or be otherwise related to #32?

(Messed up my prior comment.)

badlamer commented 7 years ago

@aviflax

Just solves this problem

BatuhanW commented 7 years ago

@badlamer Thanks for this! It was blocking us during upgrading rails version. For now I pointed it to your fork and it's working fine !

Galathius commented 7 years ago

👍

subos2008 commented 7 years ago

As much as I want to see this fixed I'm not sure this commit preserves the gem's intended behaviour.

i.e. from the updated tests:

let(:result) { TimeDifference::SI_BASE_UNIT ? "1 Year, 1 Month, 4 Weeks, 2 Days, 7 Hours, 41 Minutes and 42 Seconds" : "1 Year, 2 Months and 18 Hours" }

Originally the output would be a succinct:

1 Year, 2 Months and 18 Hours

With this update the output would be:

1 Year, 1 Month, 4 Weeks, 2 Days, 7 Hours, 41 Minutes and 42 Seconds

This kind of misses the point of .humanize in my opinion. Though it is an improvement from the status quo of being completely broken.

tmlee commented 6 years ago

Thanks for the proposal, I understand that this fix has been quite useful before the fix in the latest version of Activesupport.

While this change appear like a workaround and changes the behavior slightly; are you guys still using this change as a workaround for your problem; or did you proceed by updating ActiveSupport?

Update: After looking at the discussion again. I think 5.1 support is required. Because the way time calculation works have changed. I think we should have the latest version worked for 5.1, and a subversion to continue supporting prior to 5.1

tmlee commented 6 years ago

@badlamer Thank you for your workaround. I would like to accept the PR; but I think the best way to go would be to not support multiple version of ActiveSupport within the same gem. Here is where I think the direction of this gem should go.

  1. Keep the old version to continue to work with older version of Activesupport as expected https://github.com/tmlee/time_difference/tree/0.6.0-activesupport42

  2. The current bleeding edge will drop support from older version of Activesupport. The way seconds are calculated in 5.1 has changed. And thus I am updating the humanize to pad up in order to prevent it from having too many small minutes and seconds. After all it is used as a humanized way to represent the time difference; with slight inaccuracy.