Skip to content

DOC: add checks on the returns section in the docstrings (#23138) #23432

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 13 commits into from
Dec 30, 2018
Merged

DOC: add checks on the returns section in the docstrings (#23138) #23432

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e0f9689
DOC: add checks on the returns section in the docstrings (#23138)
igorfassen Oct 31, 2018
20b32e7
Update scripts/validate_docstrings.py
datapythonista Nov 5, 2018
f2d6449
update validate_docstrings.py: clearer error message
igorfassen Nov 5, 2018
0d34a88
update test_validate_docstrings.py: fix expected error message
igorfassen Nov 5, 2018
b09c322
update validate_docstrings.py: Returns section validation
igorfassen Nov 5, 2018
a556925
Merge remote-tracking branch 'upstream/master' into validate_returns
igorfassen Nov 6, 2018
a1384d4
update validate_docstrings.py: split line to comply with pep 8
igorfassen Nov 6, 2018
2f3f5bf
update validate_docstrings.py: small fixes in Returns validation
igorfassen Nov 7, 2018
e62de14
Merge remote-tracking branch 'upstream/master' into validate_returns
igorfassen Nov 8, 2018
fdad765
update validate_docstrings.py: simplify returns section validation
igorfassen Nov 8, 2018
58a0a91
update validate_docstrings.py: replace " by ' for homogenization
igorfassen Nov 9, 2018
a116a49
Merge remote-tracking branch 'upstream/master' into validate_returns
datapythonista Dec 30, 2018
5ea7881
Minor fixes
datapythonista Dec 30, 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
38 changes: 34 additions & 4 deletions scripts/tests/test_validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,30 @@ def no_punctuation(self):
"""
return "Hello world!"

def named_single_return(self):
"""
Provides name but returns only one value.

Returns
-------
s : str
A nice greeting.
"""
return "Hello world!"

def no_capitalization(self):
"""
Forgets capitalization in return values descriptions.

Returns
-------
foo : str
The first returned string.
bar : str
the second returned string.
"""
return "Hello", "World!"


class BadSeeAlso(object):

Expand Down Expand Up @@ -755,10 +779,16 @@ def test_bad_generic_functions(self, func):
('BadReturns', 'yield_not_documented', ('No Yields section found',)),
pytest.param('BadReturns', 'no_type', ('foo',),
marks=pytest.mark.xfail),
Copy link
Member

Choose a reason for hiding this comment

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

We can test this now. I guess we can't know whether we're missing the type or the description. We should return a message that considers both, so the user always know what to do. And test this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, with the current modifications, in this case the error message is Return value has no description
should I replace it by something like Return value has no type or description ?
(well, in the case where a tuple is returned, that would mean that both type and name are missing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while writing that, I realize that I do not check, in the case of multiple return values, if all of them have both a name and a type

pytest.param('BadReturns', 'no_description', ('foo',),
marks=pytest.mark.xfail),
pytest.param('BadReturns', 'no_punctuation', ('foo',),
marks=pytest.mark.xfail),
('BadReturns', 'no_description',
('Return value has no description',)),
('BadReturns', 'no_punctuation',
('Return value description should finish with "."',)),
('BadReturns', 'named_single_return',
('The first line of the Returns section should contain only the '
'type, unless multiple values are being returned',)),
('BadReturns', 'no_capitalization',
('Return value description should start with a capital '
'letter',)),
# Examples tests
('BadGenericDocStrings', 'method',
('Do not import numpy, as it is imported automatically',)),
Expand Down
27 changes: 25 additions & 2 deletions scripts/validate_docstrings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
'capital letter',
'PR09': 'Parameter "{param_name}" description should finish with "."',
'RT01': 'No Returns section found',
'RT02': 'The first line of the Returns section should contain only the '
'type, unless multiple values are being returned',
'RT03': 'Return value has no description',
'RT04': 'Return value description should start with a capital letter',
'RT05': 'Return value description should finish with "."',
'YD01': 'No Yields section found',
'SA01': 'See Also section not found',
'SA02': 'Missing period at end of description for See Also '
Expand Down Expand Up @@ -616,8 +621,26 @@ def validate_one(func_name):
errs.append(error('PR09', param_name=param))

if doc.is_function_or_method:
if not doc.returns and 'return' in doc.method_source:
errs.append(error('RT01'))
if not doc.returns:
if "return" in doc.method_source:
errs.append(error('RT01'))
else:
if len(doc.returns) == 1 and doc.returns[0][1]:
errs.append(error('RT02'))
missing_desc = missing_cap = missing_period = False
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need this line? Couldn't we just use the assignments within the loop over doc.returns?

Copy link
Member

Choose a reason for hiding this comment

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

Or rather not even assign to variables but simply check if not desc: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll try something simpler.
my idea was to avoid duplicate error messages, so these variables allowed me to detect the first occurrence of each type of error, and then short-circuit the subsequent tests. but I must admit this was a bit less readable...

for name, type_, desc in doc.returns:
Copy link
Member

Choose a reason for hiding this comment

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

What is the value of name in the case we've just got a single return?

I'd still like to have the names of the return fields that are failing in the returned error messages, and not give the user something like:

Return value has no description
Return value has no description
Return value has no description

@WillAyd can you propose your preferred solution for this?

For me the best would be the original one:

errs.append(error('RT03', return_name='{} '.format(name) if name else ''))

I think with the way it is implemented now it should be perfectly readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when there is a single return and the first line is correctly written (ie no name is given), name contains the type, and type_ is empty

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to rename name to name_or_type then, and possibly add a with what you just said.

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 totally indifferent as I really think this is an edge case. Whatever can be done simply and accurately works for me!

desc = ''.join(desc)
missing_desc = missing_desc or not desc
missing_cap = missing_cap or desc and not desc[0].isupper()
missing_period = (missing_period
or desc and not desc.endswith('.'))
if missing_desc:
errs.append(error('RT03'))
if missing_cap:
errs.append(error('RT04'))
if missing_period:
errs.append(error('RT05'))

if not doc.yields and 'yield' in doc.method_source:
errs.append(error('YD01'))

Expand Down