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

Backport zoneinfo logic into dateutil.tz.tzfile #1130

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

pganssle
Copy link
Member

@pganssle pganssle commented May 20, 2021

Summary of changes

This moves tzfile into its own submodule containing what is effectively a backport of the pure python implementation of zoneinfo.ZoneInfo, but with the semantics of a dateutil.tz.tzfile object.

The major differences from zoneinfo are:

  1. All caching logic is implemented in tz.gettz rather than in tz.tzfile.
  2. tzfile objects are pickled by value, not by reference to the IANA key.
  3. tzfile has equality-by-value semantics, whereas zoneinfo has equality-by-identity semantics.

This does change the internal implementation details of the class. It also fixes at least two bugs:

  1. It adds support for V2 and V3 files, which also solves the 2038 problem (and the "slim zoneinfo" problem).
  2. Prior to this change, dateutil did not use the fold attribute during gaps (only during folds), in violation of PEP 495. This fixes that, but that does change the semantics of imaginary times a bit (and thus may affect some mechanisms for detecting imaginary times).

This is mostly a code dump with compatibility adjustments. It's quite possible that we'll want to refactor, particularly with respect to the way POSIX strings are handled. Theoretically tzfile could fall back to a tz.tzstr or some other tz.tzrange, or we could refactor tzstr and tzrange in terms of _CalendarOffset, _DayOffset and _TZStr (or both, by refactoring the internal logic of tzstr and/or tzrange).

Closes #462, #1059

Open Tasks:

  • Add the key parameter for consistency with zoneinfo.ZoneInfo
  • Add support for customizing TZPATH
  • Backport relevant tests from zoneinfo
    • Basic tests
    • Weird zone tests
    • TZStr-specific tests
    • TZPath tests
    • V1 support tests
  • Fix issue with the repr when falling back to tzdata
  • Add tests against the "slim" tzdata builds.
  • Format all new and changed code with black (at least trying to be incremental here when we can...)
  • Document all relevant changes

Pull Request Checklist

  • Changes have tests
  • Authors have been added to AUTHORS.md
  • News fragment added in changelog.d. See CONTRIBUTING.md for details

@mariocj89
Copy link
Member

@pganssle do you want me to review this?

@pganssle
Copy link
Member Author

pganssle commented Jun 9, 2021

@mariocj89 I still need to backport the tests, but you are welcome to review it. I expect in the long run we'll want to consolidate some of the logic around tzstr and tzrange and some of these custom rule representations built into the TZif parser (for the extended version 2 support that allows specifying a POSIX string), but I don't expect to overhaul anything else.

Also I believe some documentation changes will be necessary before we can merge this as well.

@mariocj89
Copy link
Member

OK, I'll take a look once all is ready.

@Jorricks
Copy link

Jorricks commented Jul 25, 2022

So @mariocj89 I suppose this is really the only remaining blocker for dropping support for Python 2?

Dropping support for Python 2 would mean we could get into in-place typing info and all the new fun stuff right :)?

Given this has been open for ages, should I try to prioritize this MR, open a new MR and see if we can get somewhere within the next few months?

@mariocj89
Copy link
Member

@Jorricks this is indeed blocking most of the progress in the repo. If you have cycles to work on this we could drop Python 2 after we land and release this. @pganssle has been wanting to work on this for a long time but was not able to commit time to it.

@kloczek
Copy link

kloczek commented Aug 7, 2022

+1

@Jorricks
Copy link

Jorricks commented Aug 11, 2022

Hi @mariocj89,
I'm attempting to start but I am missing some context. Could you help me out answering these questions :)?

  1. How is this effort blocking the Python2 removal/deprecation effort?
  2. If I understand correctly, the idea of this MR is to back port a copy of zoneinfo (that is only available in Python3.9+) into this application which is compatible with the current tzinfo class right?
  3. Is the idea to implement this feature Python3.7+ or should it be Python2 compatible? Mostly asking because I'd like to add type hints already :).

@mariocj89
Copy link
Member

How is this effort blocking the Python2 removal/deprecation effort?

Without this change users in 2.7 will not get tz upgrades. This splits the tzdata from the package, allowing users to stay in an old version of the package but still get tzdata upgrades.

If I understand correctly, the idea of this MR is to back port a copy of zoneinfo (that is only available in Python3.9+) into this application which is compatible with the current tzinfo class right?

Correct

Is the idea to implement this feature Python3.7+ or should it be Python2 compatible? Mostly asking because I'd like to add type hints already :).

Needs to work for py2, so no type hints :).

@Jorricks
Copy link

How is this effort blocking the Python2 removal/deprecation effort?

Without this change users in 2.7 will not get tz upgrades. This splits the tzdata from the package, allowing users to stay in an old version of the package but still get tzdata upgrades.

If I understand correctly, the idea of this MR is to back port a copy of zoneinfo (that is only available in Python3.9+) into this application which is compatible with the current tzinfo class right?

Correct

Is the idea to implement this feature Python3.7+ or should it be Python2 compatible? Mostly asking because I'd like to add type hints already :).

Needs to work for py2, so no type hints :).

Now it completely makes sense! Thanks.

@Jorricks
Copy link

Jorricks commented Aug 13, 2022

@mariocj89 to improve visibility on the progress & smoothen the process a little. Can we open a new branch containing this work so far?
We can just keep using this topic to keep track of everything that remains to be done so far I suppose.

I was diving a bit further (as I want to make sure I understand the full context before making changes) and was wondering the following:
During the building of the python-dateutil wheel, we package the dateutil-zoneinfo.tar.gz. Could(/Should?) this then also be replaced with a slim version of tzdata?

@mariocj89
Copy link
Member

@pganssle do you have some time to guide @Jorricks through this change please?

@sfc-gh-izinkovsky
Copy link

@pganssle are you still actively working on getting this change merged?

@Jorricks
Copy link

Jorricks commented Oct 5, 2022

Unfortunately I don't have the time anymore to work on this and made zero progress anyway. So I don't think anyone is working on this.

The code is very complex and you are only doing this for python 2 users. Given that reached end of life years ago, I'm not to enthusiastic to work on this anyway.

@pganssle
Copy link
Member Author

Made a lot of progress on this at the sprints, but I don't think I'm going to quite get there today. I believe the main things left to do are:

  1. Documentation of the changes
  2. Test the tzfile with V1 data
  3. Maybe adjust the repr to treat key/filename a bit better
  4. Test the situation where only slim tzdata is installed

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.

tzfile reader only reads 32-bit (verson 0) zoneinfo files
8 participants