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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7914f55
Checks for tz/tzinfo validity in Timestamp.__new__
jbrockmendel Oct 22, 2017
5c89630
Fix exception type in tests
jbrockmendel Oct 23, 2017
6a2f766
typo fixup; whatsnew note
jbrockmendel Oct 23, 2017
b4c98fb
handle tz/tzinfo in cases where they dont disagree
jbrockmendel Oct 23, 2017
8f304a6
Edits per reviewer requests
jbrockmendel Oct 23, 2017
f812e90
revert to more verbose but more correct version
jbrockmendel Oct 24, 2017
d90bc25
Merge branch 'master' of https://github.com/pandas-dev/pandas into ti…
jbrockmendel Oct 25, 2017
4839ffb
Merge branch 'master' of https://github.com/pandas-dev/pandas into ti…
jbrockmendel Oct 27, 2017
98b5b17
move whatsnew note to 0.22.0
jbrockmendel Oct 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion pandas/_libs/tslib.pyx
View file
Open in desktop
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,8 +374,24 @@ 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')
elif ts_input is not _no_input:
raise ValueError('When creating a Timestamp with this type '
Copy link
Contributor

Choose a reason for hiding this comment

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

misspell

what case is this trying to catch? this is a confusing error message

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch on the spell-check, thanks.

Under the status quo, the tzinfo kwarg is used in only one branch:

        if ts_input is _no_input:
            # User passed keyword arguments.
            return Timestamp(datetime(year, month, day, hour or 0,
                                      minute or 0, second or 0,
                                      microsecond or 0, tzinfo),
                             tz=tzinfo)

In all other cases tzinfo is silently ignored. This raises instead of ignoring the input. As you noted below, it may be better to change this behavior.

You're right the message is oddly worded. "this type of inputs" in this case is "anything other than the keyword arguments for datetime(...)". Suggestions for wording? (unnecessary if we change the behavior instead)

'of inputs, the `tzfinfo` argument is '
'ignored. Use `tz` instead.')

if ts_input is _no_input:
# User passed keyword arguments.
if tz is not None:
raise ValueError('When creating a Timestamp from component '
'elements, the `tz` argument is ignored. '
'Use `tzinfo` instead.')
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),
Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/scalar/test_timestamp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,21 @@ 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')

with tm.assert_raises_regex(ValueError, 'Use `tzinfo` instead'):
Timestamp(year=2017, month=10, day=22, tz='UTC')
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also valid, you just can't pass tzinfo thru to the datetime constructor. rather you need to construct the Timestamp, then localize.

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. So the block that currently reads:

        if ts_input is _no_input:
            # User passed keyword arguments.
            return Timestamp(datetime(year, month, day, hour or 0,
                                      minute or 0, second or 0,
                                      microsecond or 0, tzinfo),
                             tz=tzinfo)

should become something like

        if ts_input is _no_input:
            # User passed keyword arguments.
            if tz is None:
                  tz = tzinfo
            return Timestamp(datetime(year, month, day, hour or 0,
                                      minute or 0, second or 0,
                                      microsecond or 0, tzinfo),
                             tz=tz)

?


with tm.assert_raises_regex(ValueError, 'Use `tz` instead'):
dt = datetime(2017, 10, 22)
Timestamp(dt, tzinfo=utc)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a valid case. you can just do (internally).

tz, tzinfo = tzinfo, None

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm this gets really verbose since we need to check in a handful of different places. I guess get correctness right first, then circle back for brevity.


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