<achin>
sharkdp: i'd like to repond to both your comments/suggestions about using any string type for tz conversions (instead of literal string expressions), and removing the `DateOperationResult` enum thingy. do you feel like these should be done in #287, or can they be deferred to a followup PR?
<sharkdp>
"using any string type for tz conversions (instead of literal string expressions)" -> I think this is really just that one-line fix, I already tried it
<achin>
ah, super, i'll click the [Commit suggestion] button :)
<sharkdp>
I'll check if I can remove the DateOperationResult struct
<sharkdp>
fixed
<achin>
i think we clobbered each other with our respective forcepushes
<achin>
oh, no, you inluded the cargofmt fix 👍
<sharkdp>
yes, I saw that you fixed it too. But I was too lazy to properly rebase, sorry :-)
<sharkdp>
There is a "TODO check that this handles negative durations correctly" in vm.rs... did you have anything in mind that might not work?
<achin>
i wanted to make sure that "truncate away from zero" was the right operation
<achin>
and if adding the chrono::Duration::nanoseconds is right (or if we need to use subtration here) when `seconds_f` is negative
<sharkdp>
ok, so the question is: "is x.trunc() + x.frac() == x for all x"?
<sharkdp>
(that property seems to be fulfilled)
<sharkdp>
From what I can tell, this works as expected. I'll remove the TODO for now and then we can later add some tests
sharkdp has quit [Remote host closed the connection]
sharkdp has joined #numbat
sharkdp has quit [Remote host closed the connection]
<achin>
🎉 thank you sharkdp for your help getting this merged!
sharkdp has joined #numbat
<sharkdp>
achin: Thank you! The changes are now also live on https://numbat.dev/
<achin>
i'm traveling this weekend, so i probably not be around that much, but i definitely want to work on a few of these enhancements. i'll coordinate with you in here on that
<sharkdp>
Just saw that as well via the GitHub link in my PR
<sharkdp>
triallax: I followed your suggestion with the plural 's' and also changed the format to something that is parse-able by Numbat: https://github.com/sharkdp/numbat/pull/327
<sharkdp>
achin: I found two panic!s along the way while writing tests for this new 'human time duration' feature
<sharkdp>
I added two new runtime errors to handle those cases (overflows in durations and overflows in datetimes)
<achin>
i probably should have been more carefull looking at which functions have preconditions
<achin>
(i do often wish rust had tooling that could make this easier)
<sharkdp>
True. I found a LOT of these bugs in other parts of Numbat code... some of them using fuzzing.
<sharkdp>
Speaking of, let me try the fuzzer on datetime handling :-)
<achin>
yes! fuzzing is always fun
<achin>
i remember playing around with numbat fuzzing many months ago, but unfortunately it mostly found inputs that causes an overflow (which weren't interesting to me at the time), and i didn't see an obvious way to ignore those
<sharkdp>
Patch them, continue :-). But seriously: yes, it's mostly bugs that are not too exciting. I run the fuzzer with -max_len=200, because it tends to produce stack overflows in the parser otherwise with inputs like 2!!!!!!!!!….
<sharkdp>
Or a panic when "-true" was entered as input. I mean, nobody would. But I still don't want my application to panic for such simple inputs.
<achin>
i will say, though: it's really cool how the cargo-fuzz project made this type of fuzzing so easy and accessible
sharkdp has quit [Remote host closed the connection]
sharkdp has joined #numbat
<sharkdp>
iana-time-zone is already a dependency of chrono... but they only use it to get the offset, as expected.
<sharkdp>
It's pretty cool by the way that chrono and iana-time-zone have full WASM support, so they actually use JS APIs to query the timezone in the browser
sharkdp has quit [Remote host closed the connection]