Issue #390 #400
Issue #390 #400
Conversation
|
Thank you. From a quick glance, this seems to do more than just solve the problem with negative times. If so, can we please add regression tests for these other features/issues? |
|
I can - I did run the -> Human function a fair bit post changes and the tests are passing in datetime_human_tests, I assume you mean just expand the number of unit tests that are already there to something more comprehensive? |
Yes, that's generally good practice and what we expect people to do when contributing features to Numbat. :) |
|
new tests added, let me know if there are any other cases in mind that you'd like to cover |
examples/datetime_human_tests.nbt
Outdated
| assert((-1 second -> human) == "1 second ago") | ||
| assert((-25 seconds -> human) == "25 seconds ago") | ||
| assert((-49.010 seconds -> human) == "49.010 seconds ago") | ||
|
|
||
| assert((-2.5 minute -> human) == "2 minutes + 30 seconds ago") | ||
| assert((-1 hour -> human) == "1 hour ago") | ||
| assert((-3 hour -> human) == "3 hours ago") | ||
| assert((-5.34 hour -> human) == "5 hours + 20 minutes + 24 seconds ago") | ||
| assert((-7.89 hour -> human) == "7 hours + 53 minutes + 24 seconds ago") | ||
| assert((-7.89 hour - 4 minutes -> human) == "7 hours + 57 minutes + 24 seconds ago") | ||
|
|
||
| assert((-1 day -> human) == "1 day ago") | ||
| assert((-4.6 days -> human) == "4 days + 14 hours + 23 minutes ago") | ||
| assert((-9.2345 days -> human) == "9 days + 5 hours + 37 minutes + 40.800 seconds ago") | ||
| assert((-9.2345 days + 52 hours -> human) == "7 days + 1 hour + 37 minutes + 40.800 seconds ago") | ||
|
|
||
| assert((-2 weeks -> human) == "14 days ago") | ||
| assert((-4.5 weeks -> human) == "31 days + 12 hours ago") | ||
| assert((-20.5 weeks -> human) == "143 days + 12 hours ago") | ||
|
|
||
| assert((-2.5 months -> human) == "76 days + 2 hours + 12 minutes + 39.385800 seconds ago") | ||
| assert((-2.5 years -> human) == "913 days + 2 hours + 31 minutes + 52.629600003 seconds ago") | ||
| assert((-10.5 decades -> human) == "38350 days ago") | ||
|
|
||
| assert((-25 ms -> human) == "0.025 seconds ago") | ||
| assert((-6400 ms -> human) == "6.400 seconds ago") | ||
|
|
||
| assert((-2500 µs -> human) == "0.002500 seconds ago") | ||
| assert((-40525 µs -> human) == "0.040525 seconds ago") | ||
|
|
||
| assert((-5147 ns -> human) == "0.000005147 seconds ago") | ||
| assert((-23s + 1234 ns -> human) == "22.999998766 seconds ago") |
There was a problem hiding this comment.
The logic for "… ago" is relatively simple, right? In that case, we don't need to repeat all these tests here.
There was a problem hiding this comment.
True but does protect against changes if someone switches the logic in some way in the future, and with the test base's overall speed I'd say it doesn't hurt anyone to keep these in
I can remove some of the more redundant cases though if you'd prefer
There was a problem hiding this comment.
True but does protect against changes if someone switches the logic in some way in the future, and with the test base's overall speed I'd say it doesn't hurt anyone to keep these in
Speed is not the matter, but humans are going to read those tests and wonder what they are for. I would really prefer if we strip down the negative-time-duration tests to one or two.
|
|
||
| # Implementation details: | ||
| # we skip hours/minutes/seconds for durations larger than 1000 days because: | ||
| # (a) we run into floating point precision problems at the nanosecond level at this point | ||
| # (b) for much larger numbers, we can't convert to DateTimes anymore | ||
| fn human_time_check_overflow(time: Time) = |
There was a problem hiding this comment.
can we please _-prefix this as well?
|
I am still not sure what this PR brings in addition to the "… ago" handling of negative durations. For now, I'm going to close this in favor of #425. But please feel free to comment here and we can reopen it. |
For #390
Small rework of human.nbt
-1 day -> 1 day ago
-2 hours -> 2 hours ago