Skip to content

Fixed #31623 -- Allowed specifying number of adjacent time units in timesince()/timeuntil().#13145

Merged
felixxm merged 1 commit intodjango:masterfrom
timpark0807:ticket_31623
Jul 16, 2020
Merged

Fixed #31623 -- Allowed specifying number of adjacent time units in timesince()/timeuntil().#13145
felixxm merged 1 commit intodjango:masterfrom
timpark0807:ticket_31623

Conversation

@timpark0807
Copy link
Copy Markdown
Contributor

Overview

Ticket: https://code.djangoproject.com/ticket/31623

The timesince function returns the time between two datetime objects as a formatted string.

The previous implementation always returned two units of time. This update improves flexibility by allowing the user to specify the number of units to be returned. To preserve existing functionality, the default depth value is set to 2.

Example

  • depth = 1 returns "2 weeks"
  • depth = 2 returns "2 weeks, 3 days"
  • depth = 3 returns "2 weeks, 3 days, 3 hours"
  • depth = 4 returns "2 weeks, 3 days, 3 hours, 13 minutes"

Update includes:

  • New depth functionality
  • New test cases
  • Updated Doc Strings
  • Updated Release Notes

Let me know if you see anything.

Thanks!

@timpark0807
Copy link
Copy Markdown
Contributor Author

PR ready to review.

@timpark0807 timpark0807 marked this pull request as draft July 4, 2020 06:47
@timpark0807 timpark0807 marked this pull request as ready for review July 4, 2020 06:48
@smithdc1
Copy link
Copy Markdown
Member

smithdc1 commented Jul 5, 2020

Hi @timpark0807

Thanks for this. 👍

My notes:

>>> d = datetime.datetime(year=2018, month=3, day=22)
>>> timesince(d, depth=4)
'2\xa0years, 3\xa0months, 2\xa0weeks, 2\xa0days'

>>> d = datetime.datetime(year=2018, month=3, day=2)
>>> timesince(d, depth=4)
'2\xa0years, 4\xa0months'

@timpark0807
Copy link
Copy Markdown
Contributor Author

Hi @timpark0807

Thanks for this. 👍

My notes:

>>> d = datetime.datetime(year=2018, month=3, day=22)
>>> timesince(d, depth=4)
'2\xa0years, 3\xa0months, 2\xa0weeks, 2\xa0days'

>>> d = datetime.datetime(year=2018, month=3, day=2)
>>> timesince(d, depth=4)
'2\xa0years, 4\xa0months'

@smithdc1 Thanks for taking the time to look through this PR.

My responses are below:

  • Great catch on the timeuntil depth. I'll modify that to accept a pass through depth parameter.
  • Sounds good. I will work on putting those docs together once this gets approval from the community to go through.
  • The timesince function only returns adjacent time units. Because there are 0 weeks between 3/2/18 and 7/5/20, the day/hour/minute units will not be processed.

Screen Shot 2020-07-05 at 9 52 39 AM

Screenshot using http://www.mathcats.com/explore/elapsedtime.html

As a side note, I was able to implement a timesince function that would be able to skip through missing adjacent units (The example above would return 2 Years, 4 Months, 3 Days, 10 Hours) but I didn't want to mess with original functionality. I'm happy to discuss this further and create a new ticket if this type of behavior is desired.

@smithdc1
Copy link
Copy Markdown
Member

smithdc1 commented Jul 5, 2020

  • The timesince function only returns adjacent time units. Because there are 0 weeks between 3/2/18 and 7/5/20, the day/hour/minute units will not be processed.

The current function only returns a depth of two, so stopping if there isn't an adjacent time depth gives the answer I would expect. (example below)

When depth is increased to three or above there is a chance one of the depths is "0" but there are values at more granular time depths. Chopping at the first "0" I find surprising (just my opinion, I'm sure there are others!).

For completeness I think the example you posted is what would expect. 👍
This --> 2 Years, 4 Months, 3 Days, 10 Hours
Not --> 2 Years, 4 Months, 0 Weeks, 3 Days, 10 Hours

>>> d = datetime(year=2018, month=6, day=2)
>>> timesince(d)
'2\xa0years, 1\xa0month'
>>> d = datetime(year=2018, month=6, day=20)
>>> timesince(d)
'2\xa0years'

@timpark0807
Copy link
Copy Markdown
Contributor Author

When depth is increased to three or above there is a chance one of the depths is "0" but there are values at more granular time depths. Chopping at the first "0" I find surprising (just my opinion, I'm sure there are others!).

For completeness I think the example you posted is what would expect. 👍
This --> 2 Years, 4 Months, 3 Days, 10 Hours
Not --> 2 Years, 4 Months, 0 Weeks, 3 Days, 10 Hours

I 100% agree with your opinion.

However, I decided against changing the behavior within this PR because the original ticket specifically states "The existing rule of values having to be adjacent to one another should still remain."

Let's get the community opinion to see if we should proceed with a modified ticket to not chop at the first "0".

@timpark0807 timpark0807 force-pushed the ticket_31623 branch 3 times, most recently from e7c4571 to ecdf522 Compare July 7, 2020 01:39
Copy link
Copy Markdown
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@timpark0807 Thanks 👍

@felixxm felixxm self-assigned this Jul 8, 2020
@timpark0807 timpark0807 force-pushed the ticket_31623 branch 2 times, most recently from 1ee143e to 1f1116f Compare July 14, 2020 00:47
@timpark0807
Copy link
Copy Markdown
Contributor Author

@felixxm Thanks for the thorough review. I got a bit carried away with my first PR. I'll keep these comments in mind for future commits. Let me know if you see anything else 👍

@felixxm
Copy link
Copy Markdown
Member

felixxm commented Jul 14, 2020

@claudep You mentioned in the ticket and on the mailing list that the most important thing is to make it easily customizable, however I don't see how this PR solves this. I'm fine with limiting this PR to the depth argument. Do you think we should refactor timesince()?

@claudep
Copy link
Copy Markdown
Member

claudep commented Jul 14, 2020

I don't remember if I had a specific idea in mind sorry. I guess a refactor can always be done later.

@felixxm felixxm changed the title Fixed #31623 -- Improve flexibility of timesince() by adding a depth parameter Fixed #31623 -- Allowed specifying number of adjacent time units in timesince()/timeuntil(). Jul 16, 2020
@felixxm
Copy link
Copy Markdown
Member

felixxm commented Jul 16, 2020

@timpark0807 Thanks 👍 I added check for a depth value and pushed small edits. IMO we should keep the current behavior and refactoring (if necessary) can be done in a separate PR/ticket.

Copy link
Copy Markdown
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, yes, looks tidy. Thank you @timpark0807, and everyone else for reviews.

@felixxm felixxm merged commit 8fa9a6d into django:master Jul 16, 2020
@sudip358
Copy link
Copy Markdown

can you help me tp develop how old am i script similar to this?

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.

6 participants