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

tools.time: zero is a special case for seconds_to_human() #1843

Merged
merged 1 commit into from
Apr 26, 2020
Merged

Conversation

dgw
Copy link
Member

@dgw dgw commented Apr 13, 2020

Patch looks big, but that's because Python doesn't have braces. 😏 (Just kidding, I'd update the indentation even if it did. I'm not a monster.)

Description

Fixes #1841.

Setup:

>>> from sopel.tools.time import seconds_to_human

Before:

>>> seconds_to_human(0)
' ago'

After:

>>> seconds_to_human(0)
'0 seconds ago'

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
    • Caveats apply as with all my recent PRs: make quality is currently useless in my local environment
  • I have tested the functionality of the things this change touches

@dgw dgw added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Apr 13, 2020
@dgw dgw added this to the 7.0.2 milestone Apr 13, 2020
@dgw dgw requested a review from a team April 13, 2020 18:02
@Exirel
Copy link
Contributor

Exirel commented Apr 22, 2020

What about this instead?

if secs == 0:
    return 'just now'

@dgw
Copy link
Member Author

dgw commented Apr 22, 2020

@Exirel Yes in theory, but in practice it doesn't work in many of the places I and others are using this function. It's expected to give time units, and the language around it tends is often written in such a way that "just now" would be grammatically or syntactically wrong. Not sure what to do about those.

@Exirel
Copy link
Contributor

Exirel commented Apr 22, 2020

the language around it tends is often written in such a way that "just now" would be grammatically or syntactically wrong

Then I'd say help yourself and make it a function's argument with a default value (I'm suggesting this "just now" but I don't have your use-case in mind, any default will be fine by me):

def fn(secs, right_now='just now'):
    if secs == 0:
        return right_now

    # here, you can do whatever you need to do when time isn't up

@dgw
Copy link
Member Author

dgw commented Apr 22, 2020

I'll have to think about that in relation to the other future ideas (#1798, #1799, #1824) for this function.

@Exirel
Copy link
Contributor

Exirel commented Apr 22, 2020

That's only fair then. Let's keep my suggestion for later.

@dgw dgw merged commit c26a9f5 into 7.0.x Apr 26, 2020
@dgw dgw deleted the humansec_zero branch April 26, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants