From 7914f5547f7916f08fba5285cb6ac2ad8dd1855d Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Sun, 22 Oct 2017 15:49:42 -0700 Subject: [PATCH 1/7] Checks for tz/tzinfo validity in Timestamp.__new__ closes #17690 possibly #5168 --- pandas/_libs/tslib.pyx | 19 ++++++++++++++++++- pandas/tests/scalar/test_timestamp.py | 15 +++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx index a0aae6a5de707..a9de7482e5ae9 100644 --- a/pandas/_libs/tslib.pyx +++ b/pandas/_libs/tslib.pyx @@ -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, @@ -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 @@ -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 ' + '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, minute or 0, second or 0, microsecond or 0, tzinfo), diff --git a/pandas/tests/scalar/test_timestamp.py b/pandas/tests/scalar/test_timestamp.py index c1b9f858a08de..15e049a943243 100644 --- a/pandas/tests/scalar/test_timestamp.py +++ b/pandas/tests/scalar/test_timestamp.py @@ -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(TypeError, 'at most one of'): + Timestamp('2017-10-22', tzinfo=utc, tz='UTC') + + with tm.assert_raises_regex(TypeError, 'Use `tzinfo` instead'): + Timestamp(year=2017, month=10, day=22, tz='UTC') + + with tm.assert_raises_regex(TypeError, 'Use `tz` instead'): + dt = datetime(2017, 10, 22) + Timestamp(dt, tzinfo=utc) + def test_constructor_positional(self): # see gh-10758 with pytest.raises(TypeError): From 5c8963053f9a51d8d07ee69860e8bd2d261cf5a0 Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Sun, 22 Oct 2017 22:13:33 -0700 Subject: [PATCH 2/7] Fix exception type in tests --- pandas/tests/scalar/test_timestamp.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pandas/tests/scalar/test_timestamp.py b/pandas/tests/scalar/test_timestamp.py index 15e049a943243..0490c682e6769 100644 --- a/pandas/tests/scalar/test_timestamp.py +++ b/pandas/tests/scalar/test_timestamp.py @@ -180,13 +180,13 @@ def test_constructor_invalid_tz(self): with tm.assert_raises_regex(TypeError, 'must be a datetime.tzinfo'): Timestamp('2017-10-22', tzinfo='US/Eastern') - with tm.assert_raises_regex(TypeError, 'at most one of'): + with tm.assert_raises_regex(ValueError, 'at most one of'): Timestamp('2017-10-22', tzinfo=utc, tz='UTC') - with tm.assert_raises_regex(TypeError, 'Use `tzinfo` instead'): + with tm.assert_raises_regex(ValueError, 'Use `tzinfo` instead'): Timestamp(year=2017, month=10, day=22, tz='UTC') - with tm.assert_raises_regex(TypeError, 'Use `tz` instead'): + with tm.assert_raises_regex(ValueError, 'Use `tz` instead'): dt = datetime(2017, 10, 22) Timestamp(dt, tzinfo=utc) From 6a2f766a9ddf5d97a8d6638c45f120662d23b73a Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Mon, 23 Oct 2017 08:34:11 -0700 Subject: [PATCH 3/7] typo fixup; whatsnew note --- doc/source/whatsnew/v0.21.0.txt | 1 + pandas/_libs/tslib.pyx | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v0.21.0.txt b/doc/source/whatsnew/v0.21.0.txt index 18f8858748df5..7b4848a605cf9 100644 --- a/doc/source/whatsnew/v0.21.0.txt +++ b/doc/source/whatsnew/v0.21.0.txt @@ -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 `tz` or `tzinfo` arguments (:issue:`17690`) .. _whatsnew_0210.deprecations: diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx index a9de7482e5ae9..9774d6b2a496b 100644 --- a/pandas/_libs/tslib.pyx +++ b/pandas/_libs/tslib.pyx @@ -383,7 +383,7 @@ class Timestamp(_Timestamp): 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 ' - 'of inputs, the `tzfinfo` argument is ' + 'of inputs, the `tzinfo` argument is ' 'ignored. Use `tz` instead.') if ts_input is _no_input: From b4c98fb54fcba3a8a5ddfe405a445a520ee35a11 Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Mon, 23 Oct 2017 09:05:43 -0700 Subject: [PATCH 4/7] handle tz/tzinfo in cases where they dont disagree --- pandas/_libs/tslib.pyx | 18 +++++++++--------- pandas/tests/scalar/test_timestamp.py | 15 +++++++++------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx index 9774d6b2a496b..0bb3110823e32 100644 --- a/pandas/_libs/tslib.pyx +++ b/pandas/_libs/tslib.pyx @@ -381,21 +381,17 @@ class Timestamp(_Timestamp): '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 ' - 'of inputs, the `tzinfo` 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.') + if tz is None: + # This allows us to handle the case where the user passes + # `tz` and not `tzinfo`. + tz = tzinfo return Timestamp(datetime(year, month, day, hour or 0, 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[, @@ -404,6 +400,10 @@ class Timestamp(_Timestamp): year or 0, month or 0, day or 0, hour), tz=hour) + if tzinfo is not None: + # 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: diff --git a/pandas/tests/scalar/test_timestamp.py b/pandas/tests/scalar/test_timestamp.py index 0490c682e6769..9a2e5565ae41b 100644 --- a/pandas/tests/scalar/test_timestamp.py +++ b/pandas/tests/scalar/test_timestamp.py @@ -183,12 +183,15 @@ def test_constructor_invalid_tz(self): 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') - - with tm.assert_raises_regex(ValueError, 'Use `tz` instead'): - dt = datetime(2017, 10, 22) - Timestamp(dt, tzinfo=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 From 8f304a6e570f1b7fc30330f6ce0b0a57ffdfb80d Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Mon, 23 Oct 2017 16:47:12 -0700 Subject: [PATCH 5/7] Edits per reviewer requests --- doc/source/whatsnew/v0.21.0.txt | 2 +- pandas/_libs/tslib.pyx | 13 ++++--------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/doc/source/whatsnew/v0.21.0.txt b/doc/source/whatsnew/v0.21.0.txt index 7b4848a605cf9..ea0268caf0ff7 100644 --- a/doc/source/whatsnew/v0.21.0.txt +++ b/doc/source/whatsnew/v0.21.0.txt @@ -830,7 +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 `tz` or `tzinfo` arguments (:issue:`17690`) +- :class:`Timestamp` will no longer silently ignore unused or invalid `tz` or `tzinfo` arguments (:issue:`17690`) .. _whatsnew_0210.deprecations: diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx index 0bb3110823e32..2183168c304b3 100644 --- a/pandas/_libs/tslib.pyx +++ b/pandas/_libs/tslib.pyx @@ -381,16 +381,15 @@ class Timestamp(_Timestamp): 'not %s' % type(tzinfo)) elif tz is not None: raise ValueError('Can provide at most one of tz, tzinfo') + elif not is_integer_object(freq): + # tz takes on a special meaning in this case; leave it alone + tz, tzinfo = tzinfo, None if ts_input is _no_input: # User passed keyword arguments. - if tz is None: - # This allows us to handle the case where the user passes - # `tz` and not `tzinfo`. - tz = tzinfo return Timestamp(datetime(year, month, day, hour or 0, minute or 0, second or 0, - microsecond or 0, tzinfo), + microsecond or 0, tz), tz=tz) elif is_integer_object(freq): # User passed positional arguments: @@ -400,10 +399,6 @@ class Timestamp(_Timestamp): year or 0, month or 0, day or 0, hour), tz=hour) - if tzinfo is not None: - # 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: From f812e90d77a875e72b40db9111238406ce5d1743 Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Tue, 24 Oct 2017 08:42:56 -0700 Subject: [PATCH 6/7] revert to more verbose but more correct version --- pandas/_libs/tslib.pyx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx index 2183168c304b3..6f534d4acd11a 100644 --- a/pandas/_libs/tslib.pyx +++ b/pandas/_libs/tslib.pyx @@ -381,15 +381,15 @@ class Timestamp(_Timestamp): 'not %s' % type(tzinfo)) elif tz is not None: raise ValueError('Can provide at most one of tz, tzinfo') - elif not is_integer_object(freq): - # tz takes on a special meaning in this case; leave it alone - tz, tzinfo = tzinfo, None 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, minute or 0, second or 0, - microsecond or 0, tz), + microsecond or 0, tzinfo), tz=tz) elif is_integer_object(freq): # User passed positional arguments: @@ -399,6 +399,10 @@ class Timestamp(_Timestamp): year or 0, month or 0, day or 0, hour), tz=hour) + if tzinfo is not None: + # 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: From 98b5b179749c5ef62d8582ea5819ef5de130450a Mon Sep 17 00:00:00 2001 From: Brock Mendel Date: Fri, 27 Oct 2017 09:08:40 -0700 Subject: [PATCH 7/7] move whatsnew note to 0.22.0 --- doc/source/whatsnew/v0.21.0.txt | 1 - doc/source/whatsnew/v0.22.0.txt | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/source/whatsnew/v0.21.0.txt b/doc/source/whatsnew/v0.21.0.txt index 944dac229e304..4c460eeb85b82 100644 --- a/doc/source/whatsnew/v0.21.0.txt +++ b/doc/source/whatsnew/v0.21.0.txt @@ -902,7 +902,6 @@ 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`) .. _whatsnew_0210.deprecations: diff --git a/doc/source/whatsnew/v0.22.0.txt b/doc/source/whatsnew/v0.22.0.txt index 53b052a955b45..0d177f193b68e 100644 --- a/doc/source/whatsnew/v0.22.0.txt +++ b/doc/source/whatsnew/v0.22.0.txt @@ -40,7 +40,7 @@ Backwards incompatible API changes Other API Changes ^^^^^^^^^^^^^^^^^ -- +- :class:`Timestamp` will no longer silently ignore unused or invalid `tz` or `tzinfo` arguments (:issue:`17690`) - -