-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: Corrects 'reindex_axis' docstring #24105
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
The docstring included a 'tolerance' parameter which is missing from the source. This has been removed. The docstring did not describe the 'fill_value' parameter. This has been added, mostly reusing the wording in the summary part of the existing docstring.
Corrects 'reindex_axis' docstring.
Hello @eahogue! Thanks for updating the PR.
Comment last updated on December 08, 2018 at 23:57 Hours UTC |
Shortened description line of 'fill_value' in 'reindex_axis'.
Codecov Report
@@ Coverage Diff @@
## master #24105 +/- ##
==========================================
- Coverage 92.21% 92.2% -0.01%
==========================================
Files 161 161
Lines 51684 51684
==========================================
- Hits 47658 47657 -1
- Misses 4026 4027 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24105 +/- ##
==========================================
- Coverage 92.2% 92.2% -0.01%
==========================================
Files 162 162
Lines 51700 51700
==========================================
- Hits 47671 47670 -1
- Misses 4029 4030 +1
Continue to review full report at Codecov.
|
Thanks for the fix @eahogue Do you mind running I think a short summary is missing (a one liner summary), the If you don't mind fixing those, and anything else reported by the script, or that you see, that would be great. So we leave this docstring perfect. |
Added periods, reformatted summary, and other changes required by validate_docstrings.py.
Thanks, @datapythonista. I have made additional changes and hope I have committed etc correctly. The validation script no longer returns errors. PTAL. |
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.
Thanks @eahogue
Looking much better, I added some final comments. If you don't mind addressing those too, and I think we're ready to go.
Co-Authored-By: eahogue <[email protected]>
Co-Authored-By: eahogue <[email protected]>
Co-Authored-By: eahogue <[email protected]>
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.
@datapythonista ptal
|
||
By default, places NaN in locations having no value in the | ||
previous index. A new object is produced unless the new index | ||
is equivalent to the current one and copy=False. | ||
|
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.
reindex_axis
is a deprecated method, so it would be good to add a
.. deprecated:: 0.21.0
Use `reindex` instead.
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.
Thanks, I have added this to the end of the summary. Based on other docstrings it looks like that's the place to put it.
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, that looks good!
can you merge master |
@eahogue can you fix the conflicts Also, the deprecation directive should be places between the short and the extended summary, seems like we're doing it wrong in most places: https://numpydoc.readthedocs.io/en/latest/format.html#sections I create #24143to fix the remaining errors. |
Sorry for the basic question, but the tests have passed but I am not sure what else needs to be done. Have the changes gone through yet? |
I think all comments were addressed, so nothing needed to be done any more (except someone merging it). @eahogue Thanks a lot! |
The docstring included a 'tolerance' parameter which is missing from the source. This has been removed.
The docstring did not describe the 'fill_value' parameter. This has been added, mostly reusing the wording in the summary part of the existing docstring.
git diff upstream/master -u -- "*.py" | flake8 --diff