de-privatize timezone functions #17543
Conversation
gfyoung
added
Clean
Internals
pandas/_libs/tslib.pyx
Outdated
| _p_tz_cache_key, dst_cache, | ||
| _unbox_utcoffsets, | ||
| _dateutil_gettz | ||
| p_tz_cache_key, dst_cache, |
There was a problem hiding this comment.
are all of these actually used in tslib or just other modules import them? if so then just import directly from time zones
Codecov Report
@@ Coverage Diff @@
## master #17543 +/- ##
==========================================
- Coverage 91.25% 91.23% -0.03%
==========================================
Files 163 163
Lines 49606 49627 +21
==========================================
+ Hits 45266 45275 +9
- Misses 4340 4352 +12
Continue to review full report at Codecov.
|
|
linting issue. |
pandas/_libs/tslibs/timezones.pyx
Outdated
| @@ -111,16 +111,16 @@ cpdef inline object maybe_get_tz(object tz): | |||
| return tz | |||
|
|
|||
|
|
|||
| def _p_tz_cache_key(tz): | |||
| def p_tz_cache_key(tz): | |||
| """ Python interface for cache function to facilitate testing.""" | |||
There was a problem hiding this comment.
this is dumb that we are exposing a function just to test. I don't think this is actually necessary and should be removed (so maybe leave this one as private).
There was a problem hiding this comment.
Yah, best guess is that was created before cpdef was an option. Will re-privatize.
| @@ -100,7 +100,7 @@ cpdef inline object maybe_get_tz(object tz): | |||
| tz = _dateutil_tzlocal() | |||
| elif tz.startswith('dateutil/'): | |||
| zone = tz[9:] | |||
| tz = _dateutil_gettz(zone) | |||
| tz = dateutil_gettz(zone) | |||
There was a problem hiding this comment.
so would like to rename these to be more consistent, maybe
get_timezone_from_dateutil
ok with doing it in this PR
| @@ -325,7 +325,7 @@ def test_month_range_union_tz_pytz(self): | |||
| def test_month_range_union_tz_dateutil(self): | |||
| tm._skip_if_windows_python_3() | |||
|
|
|||
| from pandas._libs.tslib import _dateutil_gettz as timezone | |||
| from pandas._libs.tslibs.timezones import dateutil_gettz as timezone | |||
There was a problem hiding this comment.
this is not great renaming
There was a problem hiding this comment.
though it might be because of generic usage in the function, maybe as get_timezone would be better
There was a problem hiding this comment.
It's in a test file. Why not just import it without an alias (i.e. just dateutil_gettz)?
There was a problem hiding this comment.
that's fine too (and better).
pandas/tests/scalar/test_period.py
Outdated
| @@ -245,8 +245,8 @@ def test_timestamp_tz_arg(self): | |||
| assert p.tz == exp.tz | |||
|
|
|||
| def test_timestamp_tz_arg_dateutil(self): | |||
| from pandas._libs.tslib import _dateutil_gettz as gettz | |||
| from pandas._libs.tslib import maybe_get_tz | |||
| from pandas._libs.tslibs.timezones import dateutil_gettz as gettz | |||
There was a problem hiding this comment.
if these can be easily changed to remove the aliasing I would do it as well (and in other locations below)
|
thanks! |
Follow-up to #17526
git diff upstream/master -u -- "*.py" | flake8 --diff