Skip to content

Fixes issues with timezone #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

philolo1
Copy link
Contributor

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.

This pr should fix the issue with the timezones uutils#36. Now all tests are passing.
@tertsdiepraam
Copy link
Member

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
Copy link
Member

tertsdiepraam commented Aug 27, 2023

Here's the culprit for at least one of them:

let time_now = Local::now().date_naive();

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: #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
Copy link
Contributor

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

@philolo1
Copy link
Contributor Author

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

@philolo1 philolo1 closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants