DOC: Deprecation directive in Docstrings should be placed before the extended summary #24188

aqurilla · 2018-12-09T23:33:51Z

closes DOC: Deprecation directive in docstrings should be placed before the extended summary #24143
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Two methods still show errors due to their docstrings being generated by the deprecate method in _decorators.py. I am not sure if that is to be changed.

pep8speaks · 2018-12-09T23:34:03Z

Hello @aqurilla! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/generic.py !
There are no PEP8 issues in the file pandas/core/indexes/base.py !
There are no PEP8 issues in the file pandas/core/indexes/multi.py !
There are no PEP8 issues in the file pandas/core/series.py !
There are no PEP8 issues in the file pandas/io/stata.py !
There are no PEP8 issues in the file scripts/tests/test_validate_docstrings.py !
There are no PEP8 issues in the file scripts/validate_docstrings.py !

datapythonista

looks great, just couple of minor things

datapythonista · 2018-12-09T23:46:31Z

scripts/tests/test_validate_docstrings.py

+        This is an extended summary with more details about
+        what `some_old_method` does.
+
+        The extended summary can contain multiple paragraphs.


Do you mind of instead of this description, explain what is wrong in the PR here.

Have changed the description to explain what is wrong with the Docstring

datapythonista · 2018-12-09T23:47:46Z

scripts/validate_docstrings.py

@@ -624,6 +625,10 @@ def get_validation_data(doc):
        errs.append(error('GL07',
                          correct_sections=', '.join(correct_order)))

+    if (doc.deprecated and not doc.name.startswith('pandas.Panel')


Are there many wrong cases in Panel? if there are just few, I'd prefer to also fix them than hardcoding this here.

pandas/scripts/validate_docstrings.py

Lines 494 to 499 in 77278ba

@property

def deprecated(self):

pattern = re.compile('.. deprecated:: ')

return (self.name.startswith('pandas.Panel')

or bool(pattern.search(self.summary))

or bool(pattern.search(self.extended_summary)))

Actually Panel has no wrong cases - I was removing it from checks due to this above existing code, which sets all Panel related objs as deprecated.

I have changed validate_docstrings.py now so that it checks these as well. Please have a look,

codecov · 2018-12-10T00:23:23Z

Codecov Report

Merging #24188 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24188   +/-   ##
=======================================
  Coverage   92.22%   92.22%           
=======================================
  Files         162      162           
  Lines       51828    51828           
=======================================
  Hits        47798    47798           
  Misses       4030     4030

Flag	Coverage Δ
#multiple	`90.62% <ø> (ø)`	⬆️
#single	`43% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.58% <ø> (ø)`	⬆️
pandas/core/generic.py	`96.65% <ø> (ø)`	⬆️
pandas/core/indexes/base.py	`96.27% <ø> (ø)`	⬆️
pandas/core/series.py	`93.7% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040f06f...5599343. Read the comment docs.

codecov · 2018-12-10T00:23:23Z

Codecov Report

Merging #24188 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24188   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files         162      162           
  Lines       51723    51723           
=======================================
  Hits        47695    47695           
  Misses       4028     4028

Flag	Coverage Δ
#multiple	`90.61% <ø> (ø)`	⬆️
#single	`43.03% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/multi.py	`95.58% <ø> (ø)`	⬆️
pandas/core/generic.py	`96.65% <ø> (ø)`	⬆️
pandas/core/indexes/base.py	`96.27% <ø> (ø)`	⬆️
pandas/core/series.py	`93.7% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77278ba...3950b3c. Read the comment docs.

datapythonista · 2018-12-10T00:47:43Z

scripts/validate_docstrings.py

@@ -624,6 +625,12 @@ def get_validation_data(doc):
        errs.append(error('GL07',
                          correct_sections=', '.join(correct_order)))

+    pattern = re.compile('.. deprecated:: ')
+    if (bool(pattern.search(doc.summary))
+            or bool(pattern.search(doc.extended_summary))):


do you mind creating a property deprecated_with_directive to avoid repeating this code?

also, not sure why we implemented it this way (and it was possibly me), but I think '.. deprecated:: ' in (doc.summary + doc.extended_summary) is a much simpler way to do it

Sure I can do that,

I had a few doubts - did you mean to also change the current deprecated property to use the simpler '.. deprecated:: ' in (doc.summary + doc.extended_summary) implementation?

Also is there any special significance of the name deprecated_with_directive? Thanks!

the current deprecated property looks for the methods/functions deprecated with the .. deprecated:: directive, and also for anything that belongs to Panel.

So, the idea would be to have a property to check the cases deprecated with the directive (so, deprecated_with_directive could be a sensible name for it), and then the current deprecated property would use it, and also check for the Panel stuff.

Got it, thanks!

Have updated both properties in this manner, please have a look,

datapythonista

looks good, just a small detail

scripts/validate_docstrings.py

datapythonista

lgtm, thanks @aqurilla

aqurilla · 2018-12-10T15:36:14Z

closes DOC: Deprecation directive in docstrings should be placed before the extended summary #24143

tests added / passed

passes git diff upstream/master -u -- "*.py" | flake8 --diff

whatsnew entry

Two methods still show errors due to their docstrings being generated by the deprecate method in _decorators.py. I am not sure if that is to be changed.

The failing check is due to these two methods mentioned above - pandas.Series.argmax and pandas.Series.argmin whose docstrings are generated by _decorators.py. What would be the best way to resolve these?

datapythonista · 2018-12-10T15:40:56Z

@jreback can Series.argmin and Series.argmax, deprecated in 0.21.0 be removed for 0.24.0? Or arent' we removing deprecated stuff until 1.0?

datapythonista · 2018-12-10T23:19:58Z

@aqurilla I created #24215 which will fix the errors caused by argmin and argmax. Once it's merged, if you merge master into your branch, it should pass the CI.

aqurilla · 2018-12-11T01:23:50Z

Ok, thanks!

WillAyd · 2018-12-12T06:21:56Z

pandas/core/generic.py

-            ``numpy.ndarray`` method ``ptp``.
-
-            .. deprecated:: 0.24.0
+            ``numpy.ndarray`` method ``ptp``.\n\n.. deprecated:: 0.24.0


What was the reason for doing this?

That was done so that the generated docstring matches the expected format of -

Summary

.. deprecated::

Extended Summary

If the validation doesn't fail, I guess this is correct. But if you can copy the output of ./scripts/validate_docstrings.py pandas.Series.<this_method> in a comment, so we can see how the final version is, that would be great.

For the final version, running python ./scripts/validate_docstrings.py pandas.Series.ptp --errors=GL09 gives -

################################################################################ ######################## Docstring (pandas.Series.ptp) ######################## ################################################################################ Returns the difference between the maximum value and the minimum value in the object. This is the equivalent of the ``numpy.ndarray`` method ``ptp``. .. deprecated:: 0.24.0 Use numpy.ptp instead

The unchanged generic.py file fails the validation with an output like below, where there is additional whitespace before the deprecated directive (does not match required format) -

################################################################################ ######################## Docstring (pandas.Series.ptp) ######################## ################################################################################ Returns the difference between the maximum value and the minimum value in the object. This is the equivalent of the ``numpy.ndarray`` method ``ptp``. .. deprecated:: 0.24.0 Use numpy.ptp instead

Use numpy.ptp instead should be indented with four spaces (the .. deprecated:: is not ok, aligned with the summary)

It actually displays like below, I think when I pasted the text here it got reformatted -

Is this ok?

The deprecated is correctly aligned. The rest will need to be changed (the summary should fit in a single line, and the rest should go to the standard summary), but that's from the original code, and we'll take care later in a separate PR.

So, happy with it here.

scripts/validate_docstrings.py

…rder Merging the changes made in 24215, to rectify the _decorators.py behavior

datapythonista · 2018-12-14T16:34:23Z

Also, @aqurilla if you can merge master into your branch, the PR that fixed argmin and argmax has been merged, and the CI should be green now.

datapythonista · 2018-12-16T02:01:26Z

Thanks for the fixes @aqurilla

@WillAyd, feel free to open a PR to merge the deprecate methods you mentioned. I think that'll make easier to understand what you meant, than discussing in comments.

…tion in the docstrings (pandas-dev#24188)

aqurilla added 3 commits December 9, 2018 17:28

DOC: Fix GH24143

1f7f9b8

DOC: Fix GH24143, updated generic.py

0388292

Updated code_checks.sh

3950b3c

datapythonista reviewed Dec 9, 2018

View reviewed changes

datapythonista added the Docs label Dec 9, 2018

Removed hardcoded 'pandas.Panel' check, changed description in test file

dae5a76

datapythonista reviewed Dec 10, 2018

View reviewed changes

added property deprecated_with_directive

470f265

datapythonista reviewed Dec 10, 2018

View reviewed changes

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

datapythonista approved these changes Dec 10, 2018

View reviewed changes

datapythonista mentioned this pull request Dec 10, 2018

Fix docstring of functions created with the deprecate() function #24215

Merged

4 tasks

datapythonista self-assigned this Dec 11, 2018

WillAyd requested changes Dec 12, 2018

View reviewed changes

aqurilla added 2 commits December 14, 2018 11:24

Merge remote-tracking branch 'upstream/master' into fix_deprecation_o…

27811ee

…rder Merging the changes made in 24215, to rectify the _decorators.py behavior

Merged from PR24215

5599343

aqurilla force-pushed the fix_deprecation_order branch from 50d192c to 5599343 Compare December 14, 2018 16:36

datapythonista merged commit d9c814f into pandas-dev:master Dec 16, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Fix and validate that deprecation directive is in the right posi…

5801e0f

…tion in the docstrings (pandas-dev#24188)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Fix and validate that deprecation directive is in the right posi…

f82982c

…tion in the docstrings (pandas-dev#24188)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Deprecation directive in Docstrings should be placed before the extended summary #24188

DOC: Deprecation directive in Docstrings should be placed before the extended summary #24188

aqurilla commented Dec 9, 2018 •

edited by datapythonista

Loading

pep8speaks commented Dec 9, 2018

datapythonista left a comment

datapythonista Dec 9, 2018

aqurilla Dec 10, 2018

datapythonista Dec 9, 2018

aqurilla Dec 10, 2018

codecov bot commented Dec 10, 2018 •

edited

Loading

codecov bot commented Dec 10, 2018

datapythonista Dec 10, 2018

aqurilla Dec 10, 2018

datapythonista Dec 10, 2018

aqurilla Dec 10, 2018 •

edited

Loading

datapythonista left a comment

datapythonista left a comment

aqurilla commented Dec 10, 2018

datapythonista commented Dec 10, 2018

datapythonista commented Dec 10, 2018

aqurilla commented Dec 11, 2018

WillAyd Dec 12, 2018

aqurilla Dec 14, 2018

datapythonista Dec 14, 2018

aqurilla Dec 14, 2018 •

edited by datapythonista

Loading

datapythonista Dec 14, 2018

aqurilla Dec 14, 2018

datapythonista Dec 16, 2018

datapythonista commented Dec 14, 2018

datapythonista commented Dec 16, 2018

	@property
	def deprecated(self):
	pattern = re.compile('.. deprecated:: ')
	return (self.name.startswith('pandas.Panel')
	or bool(pattern.search(self.summary))
	or bool(pattern.search(self.extended_summary)))

DOC: Deprecation directive in Docstrings should be placed before the extended summary #24188

DOC: Deprecation directive in Docstrings should be placed before the extended summary #24188

Conversation

aqurilla commented Dec 9, 2018 • edited by datapythonista Loading

pep8speaks commented Dec 9, 2018

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2018 • edited Loading

Codecov Report

codecov bot commented Dec 10, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqurilla Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

aqurilla commented Dec 10, 2018

datapythonista commented Dec 10, 2018

datapythonista commented Dec 10, 2018

aqurilla commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqurilla Dec 14, 2018 • edited by datapythonista Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista commented Dec 14, 2018

datapythonista commented Dec 16, 2018

aqurilla commented Dec 9, 2018 •

edited by datapythonista

Loading

codecov bot commented Dec 10, 2018 •

edited

Loading

aqurilla Dec 10, 2018 •

edited

Loading

aqurilla Dec 14, 2018 •

edited by datapythonista

Loading