search

uutils / parse_datetime

Parses a relative time string and returns a `Duration`
MIT License
16 stars 19 forks source link

Fixes issues with timezone #37

Closed philolo1 closed 1 year ago

philolo1 commented 1 year ago

This pr should fix the issue with the timezones #36. Now all tests are passing.

I am not sure whether the logic is correct or we should rather change the tests.

tertsdiepraam commented 1 year ago

Interesting! I think the tests should be run with always the same dates and timezones as reference, so Local for tests is not quite right. Rather, it should be some random (but fixed) dates and timezones. So, I'd rather have Utc in the tests, because that's supposed to fail less in different timezones. Clearly, there's some work to be done to back up that claim though 😄

tertsdiepraam commented 1 year ago

Here's the culprit for at least one of them: https://github.com/uutils/parse_datetime/blob/ff0f99bea535c41db986d02e2c98e6cd9f285267/src/lib.rs#L203

Is it just me or is that function a bit weird? It doesn't do anything with the date being passed in during parsing. It just offsets the duration at the end, but that's a strange use case isn't it?

Edit: We're not using it in coreutils either. So I'm not sure why it's there.

Edit 2: It comes from here: https://github.com/uutils/parse_datetime/pull/2. @sylvestre what do you think, should we simplify this back to one function? I'm not sure I see the use case here. At least until we start taking the date into account for computing the actual offset like in https://github.com/uutils/parse_datetime/pull/28/files.

sylvestre commented 1 year ago

heu, not sure, sorry :) maybe it can be removed? (at least, it needs to be rebased)

philolo1 commented 1 year ago

I am closing this until we find a better solution / refactor it.