-
Notifications
You must be signed in to change notification settings - Fork 451
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
Conversation
…llow it. Run the tests for zoneinfo only if available
…ividual cases fail
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
tests/test_dates.py
Outdated
@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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@akx could you have a look at this? |
babel/dates.py
Outdated
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()) |
There was a problem hiding this comment.
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)
?
tests/test_dates.py
Outdated
@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. |
There was a problem hiding this comment.
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.
@youtux, re:
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. |
@youtux Yeah, good point. That should work (assuming, you know, that people install packages with a modern I think we should still have one more release with the current guaranteed compatibility, just to ship e.g. the new CLDR changes. |
You can't save everyone :) |
I know, but a package that still has 26,000 daily downloads on Py2 should take care of those users at least somewhat! :) |
I'm not sure it should. Popular projects like django and pytest already dropped support for python2 completely. |
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