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

Handle ZoneInfo objects in get_timezone_location, get_timezone_name #741

Merged
merged 15 commits into from Nov 10, 2020
Merged

Handle ZoneInfo objects in get_timezone_location, get_timezone_name #741

merged 15 commits into from Nov 10, 2020

Conversation

youtux
Copy link
Contributor

@youtux youtux commented Oct 11, 2020

Python 3.9 introduced ZoneInfo objects, and they are available as a backport back to python 3.6.
This PR adds support for these objects.

Fixes #740

@codecov-io
Copy link

codecov-io commented Oct 11, 2020

Codecov Report

Merging #741 into master will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #741      +/-   ##
==========================================
+ Coverage   90.99%   91.02%   +0.02%     
==========================================
  Files          24       24              
  Lines        4186     4188       +2     
==========================================
+ Hits         3809     3812       +3     
+ Misses        377      376       -1     
Impacted Files Coverage Δ
babel/dates.py 91.77% <88.88%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c4282...ef7bd82. Read the comment docs.

@pytest.mark.parametrize("timezone_getter", ["zoneinfo.ZoneInfo"], indirect=True)
def test_get_timezone_name_time_zoneinfo(timezone_getter, tzname, params, expected):
"""zoneinfo will correctly detect DST from the object.
FIXME: this test will only succeed in the winter.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

how should we address this? Should I just test with a full datetime object instead?

Copy link
Member

Choose a reason for hiding this comment

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

I... guess? Getting a timezone from just a time-of-day seems weird anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I just delete this test case?
Alternatively, I can try patching datetime.utcnow() to return a constant datetime, and test against that. But it seems that the datetime module is not patchable: https://stackoverflow.com/a/192857/714760. So it would be really hard to test this case.

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 will remove this test.

@youtux
Copy link
Contributor Author

youtux commented Oct 12, 2020

@akx could you have a look at this?

tests/test_dates.py Show resolved Hide resolved
babel/dates.py Outdated
Comment on lines 506 to 511
if hasattr(tzinfo, 'zone'): # pytz object
zone = tzinfo.zone
elif hasattr(tzinfo, 'key') and tzinfo.key is not None: # ZoneInfo object
zone = tzinfo.key
else:
zone = tzinfo.tzname(dt or datetime.utcnow())
Copy link
Member

Choose a reason for hiding this comment

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

This stanza seems to be more or less the one in the below change fragment – could they be refactored into one function like get_tz_name(tzinfo, default=None)?

@pytest.mark.parametrize("timezone_getter", ["zoneinfo.ZoneInfo"], indirect=True)
def test_get_timezone_name_time_zoneinfo(timezone_getter, tzname, params, expected):
"""zoneinfo will correctly detect DST from the object.
FIXME: this test will only succeed in the winter.
Copy link
Member

Choose a reason for hiding this comment

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

I... guess? Getting a timezone from just a time-of-day seems weird anyway.

@akx
Copy link
Member

akx commented Nov 10, 2020

@youtux, re:

When is support for these old pythons going to be removed? I frankly don't see a point in supporting EOL pythons.

Regarding this, #569 is probably still relevant. I'm afraid a lot of old code and systems rely on "just give me some Babel, I don't care if it's compatible", and if a release drops support for an older Python, breakage could be immense. I do agree we should drop support for EOL versions of Python, I just don't know what the graceful way to do it is.

@akx akx merged commit 5315341 into python-babel:master Nov 10, 2020
@youtux
Copy link
Contributor Author

youtux commented Nov 10, 2020

Regarding this, #569 is probably still relevant. I'm afraid a lot of old code and systems rely on "just give me some Babel, I don't care if it's compatible", and if a release drops support for an older Python, breakage could be immense. I do agree we should drop support for EOL versions of Python, I just don't know what the graceful way to do it is.

It should not be a problem if the new release will correctly declare which python versions are supported by using the python_requires declaration in the setup.py/setup.cfg.
pip running on python2 will just pick whatever latest version of pytz still supports that. That's how most libraries handle the transition (e.g. https://github.com/pytest-dev/pytest-factoryboy/blob/910c4b0e2086c4944164bb3f631d4ac67d9d4283/setup.py#L31)

@akx
Copy link
Member

akx commented Nov 10, 2020

@youtux Yeah, good point. That should work (assuming, you know, that people install packages with a modern pip and not e.g. a prehistoric version of, say, easy_install...).

I think we should still have one more release with the current guaranteed compatibility, just to ship e.g. the new CLDR changes.

@youtux
Copy link
Contributor Author

youtux commented Nov 10, 2020

That should work (assuming, you know, that people install packages with a modern pip and not e.g. a prehistoric version of, say, easy_install...).

You can't save everyone :)

@akx
Copy link
Member

akx commented Nov 10, 2020

I know, but a package that still has 26,000 daily downloads on Py2 should take care of those users at least somewhat! :)

@youtux
Copy link
Contributor Author

youtux commented Nov 10, 2020

I'm not sure it should. Popular projects like django and pytest already dropped support for python2 completely.
I don't think you should make it your problem to be honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_timezone_location does not work with python's ZoneInfo object
3 participants