Skip to content

BUG: Ignore versionadded directive when checking for periods at docstring end #22423

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

Merged
50 changes: 49 additions & 1 deletion pandas/tests/scripts/test_validate_docstrings.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,27 @@ def contains(self, pat, case=True, na=np.nan):
"""
pass

def mode(self, axis, numeric_only):
"""
Ensure sphinx directives don't affect checks for trailing periods.

Parameters
----------
axis : str
Sentence ending in period, followed by single directive.

.. versionchanged:: 0.1.2

numeric_only : boolean
Sentence ending in period, followed by multiple directives.

.. versionadded:: 0.1.2
.. deprecated:: 0.00.0
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about versionadded and versionchanged, but deprecated can have a description after if, for example:

          .. deprecated:: 0.21.0
              Use :func:`pandas.read_csv` instead.

And it can be even multiline. Do you mind adding a test for that? I'm not sure if this is working with the current implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Also, if you check the convert_datetime64 of to_records, there are cases where the directives come before the description. I'm happy if we consider only valid having them in one place (before or after the description). But, can we make the script generate a descriptive error for it? I guess with the current implementation we'll report that the parameter has no description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test case for multi-line descriptions.
Directive positioning is a bit more tricky. Enforcing them to be in one place would help, but the problem comes when trying to determine if text after the directive is directive description, or just generic parameter description. We need to make this distinction in order to produce a nice error message.
This is made harder by the fact that we're currently working with doc_parameters, which smooshes the whole description into one single-line string.

Copy link
Member

Choose a reason for hiding this comment

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

I think enforcement after description is fine. I think @datapythonista is correct in that it will generate an error, albeit with the wrong message. If we wanted to clean that up I'd suggest a separate PR, though @datapythonista I'll leave that decision up to you

A multiline description,
which spans another line.
"""
pass


class BadGenericDocStrings(object):
"""Everything here has a bad docstring
Expand Down Expand Up @@ -374,6 +395,31 @@ def no_description_period(self, kind):
Doesn't end with a dot
"""

def no_description_period_with_directive(self, kind):
"""
Forgets to add a period, and also includes a directive.

Parameters
----------
kind : str
Doesn't end with a dot

.. versionadded:: 0.00.0
"""

def no_description_period_with_directives(self, kind):
"""
Forgets to add a period, and also includes multiple directives.

Parameters
----------
kind : str
Doesn't end with a dot

.. versionchanged:: 0.00.0
.. deprecated:: 0.00.0
"""

def parameter_capitalization(self, kind):
"""
Forgets to capitalize the description.
Expand Down Expand Up @@ -495,7 +541,7 @@ def test_good_class(self):

@pytest.mark.parametrize("func", [
'plot', 'sample', 'random_letters', 'sample_values', 'head', 'head1',
'contains'])
'contains', 'mode'])
def test_good_functions(self, func):
assert validate_one(self._import_path( # noqa: F821
klass='GoodDocStrings', func=func)) == 0
Expand Down Expand Up @@ -531,6 +577,8 @@ def test_bad_generic_functions(self, func):
'Parameter "kind: str" has no type')),
('BadParameters', 'no_description_period',
('Parameter "kind" description should finish with "."',)),
('BadParameters', 'no_description_period_with_directive',
('Parameter "kind" description should finish with "."',)),
('BadParameters', 'parameter_capitalization',
('Parameter "kind" description should start with a capital letter',)),
pytest.param('BadParameters', 'blank_lines', ('No error yet?',),
Expand Down
10 changes: 9 additions & 1 deletion scripts/validate_docstrings.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@


PRIVATE_CLASSES = ['NDFrame', 'IndexOpsMixin']
DIRECTIVES = ['versionadded', 'versionchanged', 'deprecated']


def _load_obj(obj_name):
Expand Down Expand Up @@ -234,7 +235,14 @@ def parameter_type(self, param):
return self.doc_parameters[param][0]

def parameter_desc(self, param):
return self.doc_parameters[param][1]
desc = self.doc_parameters[param][1]
# Find and strip out any sphinx directives
for directive in DIRECTIVES:
full_directive = '.. {}'.format(directive)
if full_directive in desc:
# Only retain any description before the directive
desc = desc[:desc.index(full_directive)]
return desc

@property
def see_also(self):
Expand Down