Minor update to common.format_seconds

[delirious-lettuce]

@micahflee ,

I used divmod to declutter and simplify the calculation of days, hours, minutes and seconds. Importing math becomes unnecessary (although I didn't remove the math import yet though since my other PR #413 rearranges the imports to one line each and I didn't want a merge conflict).

I made one other change. The current implementation of format_seconds returns an empty string if seconds == 0. Although the odds are quite small of that actually happening, I thought 0s might fit better with the theme of "human-readable". That is what the extra check in if seconds or not human_readable is for.

---

I've actually been writing tests for these last couple functions I've sent PR's for. I didn't want to push them yet since they were using pytest specific tests. Example:

@pytest.mark.parametrize('test_input,expected', [
    (0, '0s'),
    (26, '26s'),
    (60, '1m'),
    (1847, '30m47s'),
    (3600, '1h'),
    (16293, '4h31m33s'),
    (86400, '1d'),
    (129674, '1d12h1m14s'),
])
def test_format_seconds(test_input, expected):
    assert common.format_seconds(test_input) == expected

Instead of writing one function per test, parametrize helps to cut down on verbosity. As you probably know, stacking multiple asserts inside one regular test function has the disadvantage of stopping after the first fail and not running any tests left below it. Also, less helpful error messages.

Anyways, if you approve of using pytest then I would gladly send a PR with these types of tests.

[mig5]

With this change I get a bit of a strange decimal-like ETA that has XX.0mYs in it where XX is the minutes, and Y second

Without this change, I get a more human readable ETA like '20m53s'

[delirious-lettuce]

@mig5 ,

My mistake! Thanks for pointing this out!

I was thinking that divmod always gives back two integers but if one (or both) of the numbers is a float, divmod returns both as floats.

>>> divmod(10, 3)
(3, 1)
>>> divmod(10.0, 3)
(3.0, 1.0)
>>> divmod(10.0, 3.0)
(3.0, 1.0)

I will fix this momentarily (and then add a couple more tests to my other branch to ensure this cannot happen again).

EDIT:

Fix was a simple formatting change from {} -> {:.0f} for the days, hours, and minutes .

[mig5]

👍 tested and it looks good

[delirious-lettuce]

@mig5 ,

Thanks again for pointing this out! I guess the only issue (I'm going back and forth on the tests) is that using the string formatting rounds the number (using bankers rounding).

>>> '{:.0f}'.format(4.5)
'4'
>>> '{:.0f}'.format(5.5)
'6'
# Same idea as using `round`...
>>> round(4.5)
4
>>> round(5.5)
6

It's a small thing but maybe @micahflee 's approach with using '{}d'.format(int(days)) might be preferable? That way it just floors the result the same way each time. But then again, isn't 1.9 closer to 2 for an estimate?

>>> int(1.2)
1
>>> int(1.9)
1

Either way, I think it's only going to affect the seconds so the difference should be negligible. There are a few tests that end up being one second different and I can change them according to whatever the decision is.

---

I still think using divmod to do the calculations is much cleaner, I'm just going back and forth about how to handle the numbers.

[micahflee]

Nice, I just tested it and confirmed that it works great

[delirious-lettuce]

@micahflee ,

Awesome! I'm still working on the tests for common.py but I will update them to reflect these changes.