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

Use zoneinfo for timezone handling instead of pytz #1179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saranti
Copy link
Contributor

@saranti saranti commented Jul 7, 2024

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

  • 🧪 Added tests for changed code.
  • 🛠️ All tests pass when run locally (run tox or make test to find out!).
  • 🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!).
  • 📚 Updated documentation for changed code.
  • ⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Closes #1175. Remove all pytz code and replace it with the equivalent from zoneinfo and use a backport for python versions <3.9.

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (87a1a77) to head (d3fe586).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##            master    #1179      +/-   ##
===========================================
- Coverage   100.00%   99.91%   -0.09%     
===========================================
  Files           10       10              
  Lines         2326     2324       -2     
  Branches       400      400              
===========================================
- Hits          2326     2322       -4     
- Misses           0        1       +1     
- Partials         0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pyproject.toml Outdated
@@ -46,7 +46,6 @@ test = [
"pytest",
"pytest-cov",
"pytest-mock",
"pytz==2021.1",
"simplejson==3.*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to add a dependency on the backport like this:

backports.zoneinfo = {version = "0.2.1", markers = "python_version < '3.9'"}

from dateutil.zoneinfo import get_zonefile_instance


def make_full_tz_list():
dateutil_zones = set(get_zonefile_instance().zones)
pytz_zones = set(pytz.all_timezones)
return dateutil_zones.union(pytz_zones)
zoneinfo_zones = set(zoneinfo.available_timezones())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have the proper coverage for all timezones? We primarily used pytz to get greater coverage of time zones for the sake of tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just compared them and it does have the same coverage as pytz (Windows 11). However, from the zoneinfo docs:

The zoneinfo module does not directly provide time zone data, and instead pulls time zone information from the system time zone database or the first-party PyPI package tzdata, if available. Some systems, including notably Windows systems, do not have an IANA database available, and so for projects targeting cross-platform compatibility that require time zone data, it is recommended to declare a dependency on tzdata. If neither system data nor tzdata are available, all calls to ZoneInfo will raise ZoneInfoNotFoundError.

@@ -57,9 +56,9 @@ def test_init(self):
assert result._datetime == self.expected

# regression tests for issue #626
def test_init_pytz_timezone(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a separate test for pytz and another for zoneinfo? I don't think we should be removing pytz and closing the open issue for the sake of the tests. There is still a use-case for passing in a pytz timezone into Arrow, especially if someone has not yet adopted zoneinfo.

If pytz is not used in the core arrow code and only in the test code (it is only pulled in via requirements-tests.txt, I think we need another CR that focuses on adding support for Zoneinfo natively throughout rather than using dateutil, which is our primary means of doing timezones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

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.

RFE: use zoneinfo instead of pytz
2 participants