-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
separate DatetimeIndex timezone tests #19545
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19545 +/- ##
==========================================
+ Coverage 91.6% 91.63% +0.02%
==========================================
Files 150 150
Lines 48750 48750
==========================================
+ Hits 44657 44670 +13
+ Misses 4093 4080 -13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a rebase
return timedelta(0) | ||
|
||
|
||
fixed_off = FixedOffset(-420, '-07:00') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make these into fixtures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not being passed into parametrize anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, these should be then.
expected = np.repeat(np.array([3, 4, 5]), np.array([n, n, 1])) | ||
tm.assert_index_equal(idx.hour, Index(expected)) | ||
|
||
def test_dti_tz_convert_dst(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterize (can be next pass)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah I parametrized the most egregious of these, but there is bunch of cleanup like this for the next pass.
freq='L') | ||
with pytest.raises(pytz.NonExistentTimeError): | ||
dti.tz_localize(tzstr) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prob should dedupe these tz parameters into some nicely named fixtures (and push up what you can to conftest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next PR
localized_is_dst = dr.tz_localize(tz, ambiguous=is_dst) | ||
tm.assert_index_equal(localized, localized_is_dst) | ||
|
||
# TODO: belongs outside tz_localize tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah move to construction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the "FIXME" note about 4 lines down. I'm not confident this test is testing what it is intending. This is where the existing indentation typo is: https://github.com/pandas-dev/pandas/pull/19545/files#diff-f67205b3c64b63fa7471c4221e413a19L424
|
||
# ------------------------------------------------------------ | ||
# DatetimeIndex.__new__ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm maybe should be in construction
tm.assert_index_equal(idx1, other) | ||
|
||
# ------------------------------------------------------------ | ||
# Arithmetic; may belong in test_arithmetic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
thanks. if you can add the above items to the master TODO list would be great. I know minor, but don't want to forget about them; at the same time no reason to hold up merging this. |
Will do. I appreciate the leeway. |
There is probably some overlap with other tests.indexes.datetimes modules. We'll get those in the next pass.