search

uutils / parse_datetime

Parses a relative time string and returns a `Duration`
MIT License
14 stars 17 forks source link

Fix parsing when the local tz offset spans across midnight #55

Closed sruggier closed 3 months ago

sruggier commented 8 months ago

This should fix #36.

sruggier commented 8 months ago

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?

tertsdiepraam commented 8 months ago

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 chrono::Duration, but a custom duration, that includes the number of months as a separate field, for example. Like this library is doing: https://docs.rs/chronoutil/latest/src/chronoutil/relative_duration.rs.html#11-14. That would work too.

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.

sruggier commented 8 months ago

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.

tertsdiepraam commented 8 months ago

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: https://github.com/uutils/parse_datetime/issues/56. Maybe, a solution would be to make this library not accept a NaiveDate as reference but a full Datetime and then add the parsed duration to that? I'll bump this library on my todo list and give it a good look today :)

sruggier commented 8 months ago

Yes, NaiveDates are part of the problem here.

sylvestre commented 3 months ago

should we close this PR ? thanks

tertsdiepraam commented 3 months ago

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.