Skip to content
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

tz-info from local fixes #678

Closed
wants to merge 6 commits into from
Closed

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Apr 13, 2022

  • Add regression and more unit tests for local time
  • Add find_local_time_type_from_local for TimeZoneRef, TransitionRule and AlternateTime
  • Set the local offset whether the timestamp is local or not

This is incomplete because at least the AlternateTime::find_local_time_type_from_local still requires more work.

@djc
Copy link
Contributor

djc commented Apr 14, 2022

Awesome work adding more tests, my changes definitely didn't have great test coverage!

I'm having a hard time reviewing these changes, I'd like to cut this up into smaller commits that fix one issue at a time. Additionally I don't think we should bring in old chrono as a dependency, that seems like a brittle solution. On the other hand, shelling out to date seems like a decent option, we could also keep some of the minimal libc wrappers as they are on my branch before switching them out for tz_info and use those. What do you think?

@esheppa
Copy link
Collaborator Author

esheppa commented Apr 17, 2022

I'd like to cut this up into smaller commits that fix one issue at a time.

No problems - tests are almost passing and I need to do a few more cleanups, then will close this and split this out as much as possible.

Additionally I don't think we should bring in old chrono as a dependency

My main goal with this was to verify if there was any breakage compared to the previous version. Potentially this could be broken out to a separate crate if it is worthwhile but not ideal in the main crate? However I guess the testing against date is also essentially verifying this implementation against libc, so either one should be fine.

@djc
Copy link
Contributor

djc commented Apr 25, 2022

@esheppa have you made any progress on this? Are there things I can help with? Would be nice to get this out soon.

@esheppa
Copy link
Collaborator Author

esheppa commented Apr 25, 2022

Hi @djc - still working through this, getting closer but still a few bugs left to fix. I'm ensuring this works for both regular transitions and alternative rules for all local timezones in /usr/share/zoneinfo

@esheppa
Copy link
Collaborator Author

esheppa commented Apr 30, 2022

@djc - I'm now ready to split this into multiple PRs as the tests are passing for all but a few tzdb timezones.

There are two outstanding issues that may or may not need to be resolved, or may be able to be resolved later:

  • Which of the two zones should we pick during ambiguous (fall-back) DST transition times (where two local timezones would both be valid for a given local time). Is consistency with the date command desired here? It may also be worthwhile to add a function that returns the chrono::LocalResult so that this can be handled by users
  • Some timezones are displayed by the date command as -00:00 instead of +00:00 when the timezone isn't considered valid prior to a given start date (specifically, Factory, Antarctica/Rothera and Antarctica/Troll).

@esheppa
Copy link
Collaborator Author

esheppa commented May 1, 2022

Closing in favor of #682 and #683

@esheppa esheppa closed this May 1, 2022
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.

None yet

2 participants