Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 16, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f7f9b8
DOC: Fix GH24143
aqurilla Dec 9, 2018
0388292
DOC: Fix GH24143, updated generic.py
aqurilla Dec 9, 2018
3950b3c
Updated code_checks.sh
aqurilla Dec 9, 2018
dae5a76
Removed hardcoded 'pandas.Panel' check, changed description in test file
aqurilla Dec 10, 2018
470f265
added property deprecated_with_directive
aqurilla Dec 10, 2018
27811ee
Merge remote-tracking branch 'upstream/master' into fix_deprecation_o…
aqurilla Dec 14, 2018
5599343
Merged from PR24215
aqurilla Dec 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ci/code_checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ fi
### DOCSTRINGS ###
if [[ -z "$CHECK" || "$CHECK" == "docstrings" ]]; then

MSG='Validate docstrings (GL06, SS04, PR03, PR05, EX04)' ; echo $MSG
$BASE_DIR/scripts/validate_docstrings.py --format=azure --errors=GL06,SS04,PR03,PR05,EX04
MSG='Validate docstrings (GL09, GL06, SS04, PR03, PR05, EX04)' ; echo $MSG
$BASE_DIR/scripts/validate_docstrings.py --format=azure --errors=GL09,GL06,SS04,PR03,PR05,EX04
RET=$(($RET + $?)) ; echo $MSG "DONE"

fi
Expand Down
4 changes: 1 addition & 3 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10034,9 +10034,7 @@ def nanptp(values, axis=0, skipna=True):
cls, 'ptp', name, name2, axis_descr,
"""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
``numpy.ndarray`` method ``ptp``.\n\n.. deprecated:: 0.24.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Summary

.. deprecated::

Extended Summary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aqurilla aqurilla Dec 14, 2018 •
edited by datapythonista
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Is this ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Use numpy.ptp instead""",
nanptp)

Expand Down
6 changes: 3 additions & 3 deletions pandas/core/indexes/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2110,12 +2110,12 @@ def get_duplicates(self):
"""
Extract duplicated index elements.

Returns a sorted list of index elements which appear more than once in
the index.

.. deprecated:: 0.23.0
Use idx[idx.duplicated()].unique() instead

Returns a sorted list of index elements which appear more than once in
the index.

Returns
-------
array-like
Expand Down
4 changes: 2 additions & 2 deletions pandas/core/indexes/multi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1484,11 +1484,11 @@ def to_hierarchical(self, n_repeat, n_shuffle=1):
Return a MultiIndex reshaped to conform to the
shapes given by n_repeat and n_shuffle.

.. deprecated:: 0.24.0

Useful to replicate and rearrange a MultiIndex for combination
with another Index with n_repeat items.

.. deprecated:: 0.24.0

Parameters
----------
n_repeat : int
Expand Down
6 changes: 3 additions & 3 deletions pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,12 +1082,12 @@ def set_value(self, label, value, takeable=False):
"""
Quickly set single value at passed label.

If label is not contained, a new object is created with the label
placed at the end of the result index.

.. deprecated:: 0.21.0
Please use .at[] or .iat[] accessors.

If label is not contained, a new object is created with the label
placed at the end of the result index.

Parameters
----------
label : object
Expand Down
4 changes: 2 additions & 2 deletions pandas/io/stata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@
_data_method_doc = """\
Reads observations from Stata file, converting them into a dataframe

.. deprecated::
This is a legacy method. Use `read` in new code.
.. deprecated::
This is a legacy method. Use `read` in new code.

Parameters
----------
Expand Down
14 changes: 14 additions & 0 deletions scripts/tests/test_validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,18 @@ def sections_in_wrong_order(self):
before Examples.
"""

def deprecation_in_wrong_order(self):
"""
This docstring has the deprecation warning in the wrong order.

This is the extended summary. The correct order should be
summary, deprecation warning, extended summary.

.. deprecated:: 1.0
This should generate an error as it needs to go before
extended summary.
"""

def method_wo_docstrings(self):
pass

Expand Down Expand Up @@ -772,6 +784,8 @@ def test_bad_generic_functions(self, func):
('BadGenericDocStrings', 'sections_in_wrong_order',
('Sections are in the wrong order. Correct order is: Parameters, '
'See Also, Examples',)),
('BadGenericDocStrings', 'deprecation_in_wrong_order',
('Deprecation warning should precede extended summary',)),
('BadSeeAlso', 'desc_no_period',
('Missing period at end of description for See Also "Series.iloc"',)),
('BadSeeAlso', 'desc_first_letter_lowercase',
Expand Down
7 changes: 7 additions & 0 deletions scripts/validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
'GL07': 'Sections are in the wrong order. Correct order is: '
'{correct_sections}',
'GL08': 'The object does not have a docstring',
'GL09': 'Deprecation warning should precede extended summary',
'SS01': 'No summary found (a short summary in a single line should be '
'present at the beginning of the docstring)',
'SS02': 'Summary does not start with a capital letter',
Expand Down Expand Up @@ -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))):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@aqurilla aqurilla Dec 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

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

if not doc.extended_summary.startswith('.. deprecated:: '):
errs.append(error('GL09'))

if not doc.summary:
errs.append(error('SS01'))
else:
Expand Down