-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Implement NaT properties/methods directly #17765
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
@@ -156,6 +158,44 @@ def test_NaT_methods(): | |||
assert NaT.isoformat() == 'NaT' | |||
|
|||
|
|||
def test_NaT_docstrings(): |
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.
add the issue number as a comment
ts_expected = ['freqstr', 'normalize', 'offset', | ||
'to_julian_date', 'to_period', 'tz'] | ||
assert ts_missing == ts_expected | ||
|
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.
give a comment before each 'section' of the test so a future reader gets what you are testing
Just added comments. Note that the test implemented here is identical (except for the new comments) to the one implemented in #17756 |
Codecov Report
@@ Coverage Diff @@
## master #17765 +/- ##
==========================================
+ Coverage 91.23% 91.24% +<.01%
==========================================
Files 163 163
Lines 49869 49872 +3
==========================================
+ Hits 45499 45505 +6
+ Misses 4370 4367 -3
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.
lgtm. small comment (can defer this unless its an easy fix; if defering can you make an issue / put on todo list)
tzname = _make_error_func('tzname', datetime) | ||
utcoffset = _make_error_func('utcoffset', datetime) | ||
|
||
# Timestamp has empty docstring for some methods. |
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.
these are not defined directly in Timestamp/_Timestamp
rather directly in datetime.datetime
, maybe direct to there? (I don't know where its actually defined, nor why the inheritence messes this up)
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 are defined in Timestamp and have empty docstrings there. No idea why. Will add to todo list.
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.
ok great.
thanks! |
Causes build errors, referenced in #17756
git diff upstream/master -u -- "*.py" | flake8 --diff