Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Nice duration fix #2330

Merged
merged 2 commits into from
Oct 2, 2019
Merged

Nice duration fix #2330

merged 2 commits into from
Oct 2, 2019

Conversation

@ChanceNCounter
Copy link
Contributor

@ChanceNCounter ChanceNCounter commented Sep 27, 2019 •
edited
Loading

nice_duration heavily refactored:

  • Original logic spun off into helper function
  • New function: nice_duration_dt
    • Operates on a pair of datetime.datetimes
    • Leap-year-aware
  • No longer produces strange output on large numbers
  • New optional parameters:
    • use_years (bool): optional
    • resolution (TimeResolution): optional, see below
    • clock (bool): optional, forces output to look like digital clock (displayed output only)
  • Attempts to display "pretty" output based on resolution:
>>> nice_duration(360, resolution=TimeResolution.SECONDS)
'six minutes'
>>> nice_duration(360, resolution=TimeResolution.SECONDS, speech=False)
'6:00'
>>> nice_duration(360, resolution=TimeResolution.MINUTES, speech=False)
'6m'
>>> nice_duration(360, resolution=TimeResolution.HOURS, speech=False)
'0h'

Companion enum:
mycroft.util.format.TimeResolution
offers YEARS, DAYS, HOURS, MINUTES, SECONDS, or MILLISECONDS

Will only return ms if MILLISECONDS is chosen. Default: SECONDS

==== Fixed Issues ====
#2295

==== Tech Notes ====
Will break anything calling nice_duration() using positional arguments.
I couldn't find anything that does that, so I don't think this will break
any code in the wild.

How to test

Best to test in the interpreter. Don't forget to import mycroft.util.format.TimeResolution!
Other examples not in docstring:

>>> nice_duration(32655745.52, speech=False, resolution=TimeResolution.MILLISECONDS)
'1y 12d 23:02:25.52'
>>> nice_duration(32655745.52, speech=False, resolution=TimeResolution.MILLISECONDS, use_years=False)
'377d 23:02:25.52'
>>> nice_duration_dt(datetime(2019, 3, 12, 11, 15, 12), datetime(2019, 1, 1, 9, 7), resolution=TimeResolution.HOURS)
'seventy days two hours'

Contributor license agreement signed?

CLA [yes]

@pep8speaks
Copy link

pep8speaks commented Sep 27, 2019 •
edited
Loading

Hello @ChanceNCounter! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-02 05:58:13 UTC

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Sep 27, 2019
forslund
forslund suggested changes Sep 27, 2019
View reviewed changes
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Thanks for improving this!

I commented on a couple of small things in the code, also there is one of the test that's failing (seeTravis)

Could you add a couple of test cases with specific resolutions?

@ChanceNCounter
Copy link
Contributor Author

ChanceNCounter commented Sep 27, 2019

I'm considering the merits of rounding minutes to hours when resolution <= TimeResolution.HOURS

forslund
forslund suggested changes Sep 27, 2019
View reviewed changes
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

This seems to be working just fine. 👍

For the future we could add a TimeResolution.AUTO that sets resolution depending on the duration. But that's outside of the scope of this PR

Let's make the handle_duration a "private" function to so we can change the api if needed. Does keeping the keywords only thing make sense in that context?

@ChanceNCounter
Copy link
Contributor Author

ChanceNCounter commented Sep 29, 2019

That last commit should finally put a bow on it. I've got tests for most of the rest of util.format.py, but those can get their own PR.

@forslund
Copy link
Collaborator

forslund commented Sep 30, 2019

Thanks @ChanceNCounter, to you want to rebase and squash things into a logical order or should I just squash everything into a single commit?

@ChanceNCounter
Copy link
Contributor Author

ChanceNCounter commented Sep 30, 2019

@forslund interesting question. I think you might as well squash; I'll tag my branch, just in case, but I don't think there's any history here worth preserving.

@ChanceNCounter
Copy link
Contributor Author

ChanceNCounter commented Sep 30, 2019

One more round of edge cases coming. At least I'm finding them before the merge.

@ChanceNCounter @forslund
- make nice_duration handle years, datetimes, and ms
4253929 
- Companion enum:
mycroft.util.format.TimeResolution
offers YEARS, DAYS, HOURS, MINUTES, SECONDS, or MILLISECONDS

- Will only return ms if MILLISECONDS is chosen. Default: SECONDS

- Update tests
@ChanceNCounter @forslund
add optional 'clock' param for displayed output
8c50fe9 
Add one more optional parameter: bool clock, always produces
digital clock-like output. "0h 3m" becomes "0:03:00".

Has no effect on resolutions YEARS or DAYS, and MINUTES won't print hrs.
@forslund
Copy link
Collaborator

forslund commented Oct 2, 2019

Squashed it down. Now merging.

Big thanks @ChanceNCounter !

@forslund forslund merged commit 89741ad into MycroftAI:dev Oct 2, 2019
@ChanceNCounter ChanceNCounter deleted the nice-duration-fix branch October 2, 2019 21:52
@JarbasAl JarbasAl mentioned this pull request Oct 3, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@forslund forslund Awaiting requested review from forslund

Assignees

No one assigned

Labels

CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ChanceNCounter @pep8speaks @forslund @devs-mycroft