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

Fix tz.gettz() not returning local time for empty string #1024

Merged
merged 2 commits into from Apr 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/1024.misc.rst
@@ -0,0 +1,2 @@
Fixed tz.gettz() not returning local time when passed an empty string.
Reported by @labrys (gh issues #925, #926). Fixed by @ffe4 (gh pr #1024)
35 changes: 35 additions & 0 deletions dateutil/test/property/test_tz_prop.py
@@ -0,0 +1,35 @@
from datetime import datetime, timedelta

import pytest
import six
from hypothesis import assume, given
from hypothesis import strategies as st

from dateutil import tz as tz

EPOCHALYPSE = datetime.fromtimestamp(2147483647)
NEGATIVE_EPOCHALYPSE = datetime.fromtimestamp(0) - timedelta(seconds=2147483648)


@pytest.mark.gettz
@pytest.mark.parametrize("gettz_arg", [None, ""])
# TODO: Remove bounds when GH #590 is resolved
@given(
dt=st.datetimes(
min_value=NEGATIVE_EPOCHALYPSE, max_value=EPOCHALYPSE, timezones=st.just(tz.UTC),
)
)
def test_gettz_returns_local(gettz_arg, dt):
act_tz = tz.gettz(gettz_arg)
if isinstance(act_tz, tz.tzlocal):
return

dt_act = dt.astimezone(tz.gettz(gettz_arg))
if six.PY2:
dt_exp = dt.astimezone(tz.tzlocal())
else:
dt_exp = dt.astimezone()

assert dt_act == dt_exp
assert dt_act.tzname() == dt_exp.tzname()
assert dt_act.utcoffset() == dt_exp.utcoffset()
9 changes: 9 additions & 0 deletions dateutil/test/test_tz.py
Expand Up @@ -1079,6 +1079,15 @@ def testGettzCacheTzLocal(self):
assert local1 is not local2


@pytest.mark.gettz
def test_gettz_same_result_for_none_and_empty_string():
local_from_none = tz.gettz()
local_from_empty_string = tz.gettz("")
assert local_from_none is not None
assert local_from_empty_string is not None
assert local_from_none == local_from_empty_string


@pytest.mark.gettz
@pytest.mark.parametrize('badzone', [
'Fake.Region/Abcdefghijklmnop', # Violates several tz project name rules
Expand Down
2 changes: 1 addition & 1 deletion dateutil/tz/tz.py
Expand Up @@ -1596,7 +1596,7 @@ def nocache(name=None):
name = os.environ["TZ"]
except KeyError:
pass
if name is None or name == ":":
Copy link
Member

Choose a reason for hiding this comment

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

I'll note that this will suddenly allow tz.gettz(0) without throwing an exception.

Maybe we should go with if name is None or name in ("", ":"):?

In a potential future version we may want to explicitly raise TypeError if name is not an instance of str or None, but I'm inclined to defer the discussion about duck typing/leniency and just implement this in such a way that we don't have to address it right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, makes sense. Although I would be interested to hear what you think about how we should do typing (#383) after the deprecation #653

Also, should I squash my commits to clean up the commit history, will you just squash them all when merging, or what are your preferences in that regard?

Copy link
Member

Choose a reason for hiding this comment

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

I personally rewrite my history so that each commit is an atomic change where the tests pass. If that's too much of a pain, squashing it down is also a valid approach.

if name is None or name in ("", ":"):
for filepath in TZFILES:
if not os.path.isabs(filepath):
filename = filepath
Expand Down