Fix parsing when the local tz offset spans across midnight #55
Conversation
Previously, the implementation lived in parse_relative_time_at_date, but the "at_date" part is buggy when the local and UTC times have different dates, so removing the dependency on that code from parse_relative_time fixes a bug related to that.
This was an internal-only function, which is now not called by anything, so it can be safely removed.
sruggier
force-pushed
the
fix-local-tz-offsets-across-midnight
branch
from
2375d7d to
2786540
Compare
There was a problem hiding this comment.
I understand what you're trying to do, but I don't think it's entirely correct in the long run. The problem is that we cannot return just a chono::Duration because the length of the duration might depend on the date. For instance, if a duration of 1 month is given, that cannot be accurately represented in a chrono::Duration, because that only stores seconds and nanoseconds.
I think the calculation in parse_relative_time_at_date can probably be improved though.
|
I haven't changed the type returned by the function, so I don't think it's possible for this change to create a regression related to longer durations. Would you mind giving me a more concrete example of an input that wouldnt be handled correctly, to help me understand what you're referring to? |
|
So, you're right, you've not regressed anything, but it would've if the rest up the code was correct (and obviously that's on us 😄). Basically, what the current main branch is currently doing, for example, is assuming that a month is 30 days. That's just wrong. It reality, the number of days that a month takes depends on the reference date. However, there is an alternative solution that we could explore. Which is where we don't return just a Sorry, this library is in a pretty weird state at the moment, because the original implementation did not foresee issues like this. I should sit down and really fix it up at some point. |
|
So, how do you want to proceed? I think it would be reasonable to merge this as-is, and open an issue to track the other problem. One can always revert the test removals later on if they'd be useful after making the later change. If you'd prefer to fix both problems at the same.time, though, then it would be good to confirm that. |
|
No, sorry, I really can't merge this as is, because I've been really trying to push this library in the right direction (always having a reference datetime) and then this does not make sense. It's all related to the issues summarized here: #56. Maybe, a solution would be to make this library not accept a |
|
Yes, NaiveDates are part of the problem here. |
|
should we close this PR ? thanks |
|
Thanks for your work on this! Feel free to reopen this or open a new PR if you think that we can give this a second shot. |
This should fix #36.