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.

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 9 commits into from
Aug 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ac1ca24
BUG: Ignore versionadded directive when checking for periods at docst…
bengineer19 Aug 19, 2018
b06cdbf
TST: Added test cases for versionadded directive
bengineer19 Aug 19, 2018
25382b9
BUG: Refactor to ignore versionchanged and deprecated directives.
bengineer19 Aug 19, 2018
cd861fe
TST: Add multiple directives to tests
bengineer19 Aug 19, 2018
cb4bcc3
CLN: Add property for parameter descriptions excluding directives
bengineer19 Aug 20, 2018
748b167
TST: Update test cases for ignoring directives to be more concise.
bengineer19 Aug 20, 2018
37da2cc
CLN: Change parameter_desc to strip out directives
bengineer19 Aug 21, 2018
ccfa4d6
CLN: Remove raw_parameter_desc as it currently serves no purpose
bengineer19 Aug 22, 2018
3039d78
TST: Test directive descriptions
bengineer19 Aug 22, 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
48 changes: 47 additions & 1 deletion pandas/tests/scripts/test_validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,25 @@ 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

"""
pass


class BadGenericDocStrings(object):
"""Everything here has a bad docstring
Expand Down Expand Up @@ -374,6 +393,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 +539,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 +575,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
13 changes: 12 additions & 1 deletion scripts/validate_docstrings.py
View file
Open in desktop
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 @@ -233,9 +234,19 @@ def correct_parameters(self):
def parameter_type(self, param):
return self.doc_parameters[param][0]

def parameter_desc(self, param):
def raw_parameter_desc(self, param):
Copy link
Member

Choose a reason for hiding this comment

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

@datapythonista I know you wanted this but I would prefer not to include unless it serves a purpose. May even be better served as a keyword argument going forward instead of a dedicated method so would rather not go down this path until needed.

Outside of that I think this change looks good

Copy link
Member

Choose a reason for hiding this comment

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

Sure. We can just have parameter_desc for now, and see when we start using the directives what makes more sense.

return self.doc_parameters[param][1]

def parameter_desc(self, param):
desc = self.raw_parameter_desc(param)
# 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):
return collections.OrderedDict((name, ''.join(desc))
Expand Down