Skip to content

Checks for tz/tzinfo validity in Timestamp.__new__ #17943

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

Merged
merged 9 commits into from
Oct 27, 2017
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.21.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,7 @@ Other API Changes
- Restricted DateOffset keyword arguments. Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior. Now, only valid arguments will be accepted. (:issue:`17176`).
- Pandas no longer registers matplotlib converters on import. The converters
will be registered and used when the first plot is draw (:issue:`17710`)
- :class:`Timestamp` will no longer silently ignore unused or invalid `tz` or `tzinfo` arguments (:issue:`17690`)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move to 0.22.0 same section


.. _whatsnew_0210.deprecations:

Expand Down
20 changes: 18 additions & 2 deletions pandas/_libs/tslib.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ from util cimport (is_integer_object, is_float_object, is_datetime64_object,
is_timedelta64_object, INT64_MAX)
cimport util

from cpython.datetime cimport PyTZInfo_Check
# this is our datetime.pxd
from datetime cimport (
pandas_datetimestruct,
Expand Down Expand Up @@ -68,7 +69,7 @@ from .tslibs.parsing import parse_datetime_string

cimport cython

from pandas.compat import iteritems, callable
from pandas.compat import iteritems

import collections
import warnings
Expand Down Expand Up @@ -373,12 +374,23 @@ class Timestamp(_Timestamp):
FutureWarning)
freq = offset

if tzinfo is not None:
if not PyTZInfo_Check(tzinfo):
# tzinfo must be a datetime.tzinfo object, GH#17690
raise TypeError('tzinfo must be a datetime.tzinfo object, '
'not %s' % type(tzinfo))
elif tz is not None:
raise ValueError('Can provide at most one of tz, tzinfo')

if ts_input is _no_input:
# User passed keyword arguments.
if tz is None:
# Handle the case where the user passes `tz` and not `tzinfo`
tz = tzinfo
return Timestamp(datetime(year, month, day, hour or 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

you should set tzinfo to be None here

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

minute or 0, second or 0,
microsecond or 0, tzinfo),
tz=tzinfo)
tz=tz)
elif is_integer_object(freq):
# User passed positional arguments:
# Timestamp(year, month, day[, hour[, minute[, second[,
Expand All @@ -387,6 +399,10 @@ class Timestamp(_Timestamp):
year or 0, month or 0, day or 0,
hour), tz=hour)

if tzinfo is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be necessary if you do as the above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? Changing line 390 above doesn't affect anything at this point in the func.

Copy link
Contributor

Choose a reason for hiding this comment

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

you have the exact same check above (if tzinfo is not none), so you shoul do this up there. (line 377)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. That requires conditioning on elif not is_integer_object(freq). Just pushed.

# User passed tzinfo instead of tz; avoid silently ignoring
tz, tzinfo = tzinfo, None

ts = convert_to_tsobject(ts_input, tz, unit, 0, 0)

if ts.value == NPY_NAT:
Expand Down
18 changes: 18 additions & 0 deletions pandas/tests/scalar/test_timestamp.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,24 @@ def test_constructor_invalid(self):
with tm.assert_raises_regex(ValueError, 'Cannot convert Period'):
Timestamp(Period('1000-01-01'))

def test_constructor_invalid_tz(self):
# GH#17690, GH#5168
with tm.assert_raises_regex(TypeError, 'must be a datetime.tzinfo'):
Timestamp('2017-10-22', tzinfo='US/Eastern')

with tm.assert_raises_regex(ValueError, 'at most one of'):
Timestamp('2017-10-22', tzinfo=utc, tz='UTC')

def test_constructor_tz_or_tzinfo(self):
# GH#17943, GH#17690, GH#5168
stamps = [Timestamp(year=2017, month=10, day=22, tz='UTC'),
Timestamp(year=2017, month=10, day=22, tzinfo=utc),
Timestamp(year=2017, month=10, day=22, tz=utc),
Timestamp(datetime(2017, 10, 22), tzinfo=utc),
Timestamp(datetime(2017, 10, 22), tz='UTC'),
Timestamp(datetime(2017, 10, 22), tz=utc)]
assert all(ts == stamps[0] for ts in stamps)

def test_constructor_positional(self):
# see gh-10758
with pytest.raises(TypeError):
Expand Down