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

Using only months and days drops weeks and produces surprising results #77

Open
jmperez127 opened this issue Aug 19, 2016 · 8 comments
Open

Comments

@jmperez127
Copy link

Hello everyone!
I'm trying to use only months and days, but the value I'm getting is not right. If I set the date to 9/28 I get "1 month and 2 days" instead I should get "1 month and 9 days". So I guess it's omitting the week and is not combining the week into days. Am I doing something wrong or is this an issue? Thanks!

PS: here's the code
distance_of_time_in_words(date_a, date_b, false, :only => [:months, :days])

@lauranjansen
Copy link
Collaborator

lauranjansen commented Aug 22, 2016

This is working as intended (https://github.com/radar/dotiw#only), but maybe you can come up with an implementation of the :accumulate_on option that allows you to accumulate on multiple measurements.

@gsaks123
Copy link

2 years after this change I bumped my version of the Gem from 3.01 to the latest, and there's been a bug across my application as a result.

The application used only: [:months,:days] in many places, and so I'd previously get output like "1 month and 13 days" which I would argue in many contexts is much more readable and easily understandable than "1 month and 1 week and 6 days".

My point is that this issue raised by @jmperez127 is not actually a new feature request. The introduction of the weeks concept has resulted in a change in behavior. And, since it's by far the fastest way to address it, I'm likely to just add :weeks to my options, even though I think it results in an inferior output for readability.

Anyway, I greatly appreciate anyone that's helped build and maintain this Gem. I don't mean to come off like this is a huge problem or give anyone a hard time. Just that for posterity this issue is not exactly a new feature request, so much as a change in behavior from the way the Gem had been behaving.

I'll take a look at the :accumulate_on code to see if it's within my somewhat limited skills to try and help with this, but there's a good chance I would add more hassle than help.

@dblock
Copy link
Collaborator

dblock commented Jul 2, 2020

While this works as expected I personally do think it's surprising. Does anyone have a strong use-case of only that would drop intermediate results? Maybe that shouldn't be allowed and only: be refactored into truncate_at: or something like that?

@dblock dblock changed the title Using only months and days Using only months and days drops weeks and produces surprising results Jul 2, 2020
@gsaks123
Copy link

@dblock while it might be complex, a nifty way to do it would be to drop results SMALLER than the smallest value in the only array. For example, let's say you specify:

only: [:years, :months, :days]

Here, the smallest value in the array is days. Therefore anything for hours, minutes, and seconds is dropped. However, weeks is not smaller than the smallest value in the array, so rather than being dropped, it gets "downward converted" to the next smallest specified value.

The more I think about this as I type it, the more I suspect it would be absurdly complex and not worth the effort. But since I've already typed it up, I might as well post it and perhaps it will help you brainstorm!

@dblock
Copy link
Collaborator

dblock commented Jul 13, 2020

I think what you propose is an interesting idea! I don't think it's too bad to implement though if you want to give it a shot:

  1. Build the hash normally, you have years, weeks, days, months, minutes, seconds ...
  2. From highest to lowest: if the value is excluded (not in :only or included in except:) convert down to the next value, and add to it
  3. Truncate at the lowest.

I'd start by writing a bunch of specs :)

@gsaks123
Copy link

Hi @dblock I hope to find the time at some point to give this a shot, but I'm also not that experienced of a developer, have never really contributed to a gem like this, and might take me a while to get it acceptably implemented. But hopefully I can work on it some day.

@dblock
Copy link
Collaborator

dblock commented Oct 27, 2020

Hi @dblock I hope to find the time at some point to give this a shot, but I'm also not that experienced of a developer, have never really contributed to a gem like this, and might take me a while to get it acceptably implemented. But hopefully I can work on it some day.

Cool. I would start by writing tests of what you expect the output to be, and that alone is valuable.

@tejanium
Copy link

If we could make _display_time_in_words a public method, we could use the raw hash, and further calculate any value we want

[1] pry(main)> hash = DOTIW::Methods.distance_of_time_in_words_hash(Time.now, 72.days.ago, except: [:minutes, :hours])
=> {:years=>0, :months=>2, :weeks=>1, :days=>4, :minutes=>59, :seconds=>59}

[2] pry(main)> hash = hash.merge(days: hash[:days] + (hash.delete(:weeks) || 0) * 7)
=> {:years=>0, :months=>2, :days=>11, :minutes=>59, :seconds=>59}

[3] pry(main)> DOTIW::Methods.send(:_display_time_in_words, hash)
=> "2 months, 11 days and 59 minutes"

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

No branches or pull requests

5 participants