fix use of span to get number of days #29
Conversation
This commit revises the change in Byron#28 to be a bit more robust. Originally, before Byron#28, the code was buggy because, I think, `Span` was being used like a normal "absolute" duration. But a `Span` keeps track of values for each individual unit. It isn't just a single number of nanoseconds like, e.g., `std::time::Duration` is. It's "smarter" than that. It deals with non-uniform units like days, months and years. But to do it correctly, you need a reference date. What this means is that when you get a `Span` by subtracting two `Zoned` values, you can't just ask the `Span` for the total number of days via `get_days()`. It has to be *computed*. In Byron#28, this was done for one case but not the other via the `Span::total` API. While this works today, the code was not providing a reference date, which means days are silently treated as always being 24 hours long. See BurntSushi/jiff#48 for more details where it's likely that this sort of usage will return an error in `jiff 0.2`. The main gotcha here is that since this is using `gix::date`, the `Zoned` values that are created are just "fixed offset" datetimes. They don't actually have a time zone. So in practice, such datetimes will always have all days be 24 hours long. This is not correct, but it's not clear to me that this is fixable inside the context of `git`. But at least with this patch, if you do ever end up using true time zones, then this code will be robust to that.
BurntSushi
force-pushed
the
ag/fix-day-span
branch
from
9309196
to
a108b79
Compare
|
This might also be helpful clarifying material: https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html#when-should-i-use-signedduration-versus-span |
There was a problem hiding this comment.
I am incredibly surprised and just as grateful that you saw my 'fix' on top of the previous 'hack', and feel honoured to have your correction in the history of cargo-smart-release 😊🙇♂️.
Also thanks for the additional explanation here, I spent some time understanding it and hopefully making the difference sink in. Admittedly, during my initial conversion from time to jiff I just tried to make it compile and didn't spend any time trying to understand the underlying concepts. Had I read the excellent docs of get_days() I would probably have seen the warning that it's not what you think it is :D.
And even from there, all I did was to try and find alternatives that seemed to work. Never would I have been able to arrive at the solution presented here.
When looking further into it, I noticed that Zoned::sub(Zoned) also returns a Span, but the difference seems to be that the smallest and largest unit probably default to hours. Then, when being interested in days, it probably yields incorrect results.
I hope that in future, I will be able to use jiffs excellent (and incredibly well documented) API to handle time correctly, and that should be facilitated by not coming from time but start from scratch.
Thanks again 🙏.
|
Thanks for the kind words. :-)
Ug, this is bad! I guess once Popping up a level, I spent some time last night thinking about git and its use of offset datetimes everywhere instead of time zones. It is an interesting case where perhaps time zones aren't the right thing! Consider what happens if one commit has a datetime recorded at |
I think it's entirely unrelated to the quality of the documentation though, and agree that it's mostly an issue of habit and 'most developers have no clue about time'. In my case, it was a zero-thought-quick-and-dirty-conversion where compilation alone meant success. Edit: I think what I wouldn't have done is to do the rounding, smallest and largest, that's entirely new concepts to me, strangely enough.
I suppose, one would have to choose one of the two dates, transform it to the other, and then check the amount of days that passed between the timezone-normalized dates accordingly? |
This commit revises the change in #28 to be a bit more robust.
Originally, before #28, the code was buggy because, I think,
Spanwasbeing used like a normal "absolute" duration. But a
Spankeeps trackof values for each individual unit. It isn't just a single number of
nanoseconds like, e.g.,
std::time::Durationis. It's "smarter" thanthat. It deals with non-uniform units like days, months and years. But
to do it correctly, you need a reference date.
What this means is that when you get a
Spanby subtracting twoZonedvalues, you can't just ask the
Spanfor the total number of days viaget_days(). It has to be computed. In #28, this was done for onecase but not the other via the
Span::totalAPI. While this workstoday, the code was not providing a reference date, which means days are
silently treated as always being 24 hours long.
See BurntSushi/jiff#48 for more details where
it's likely that this sort of usage will return an error in
jiff 0.2.The main gotcha here is that since this is using
gix::date, theZonedvalues that are created are just "fixed offset" datetimes. Theydon't actually have a time zone. So in practice, such datetimes will
always have all days be 24 hours long. This is not correct, but it's not
clear to me that this is fixable inside the context of
git. But atleast with this patch, if you do ever end up using true time zones, then
this code will be robust to that.