Skip to content
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

require Return section only if return is not None nor commentary #25008

Merged
merged 45 commits into from
Mar 11, 2019
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
575abc4
Return section only required if at least one return is not None nor c…
Jan 29, 2019
4b6406f
fixed PEP8 issues
Jan 29, 2019
b204f11
added another "return" on test
Jan 29, 2019
0a76a3a
now respects summary in a single line
Jan 29, 2019
d35ec84
only look for return commands after docstring
Jan 29, 2019
e7d9bed
Added comments to regex that finds non-bare return commands
Jan 30, 2019
2cdd367
added space around bitwise or operator
Jan 30, 2019
0e25d93
Replaced [ \t\f\v] for \s on regex.
Jan 31, 2019
d5d9800
Added test with valued return after bare returns
Feb 1, 2019
073a6b6
method_source now returns source without docstring
Feb 2, 2019
0143b7c
Updated to search string without whitespaces.
Feb 2, 2019
76188dd
Simplified regex.
Feb 2, 2019
2051ea6
Created clean_method_source property without comments or docstring.
Feb 2, 2019
ff17a79
Updated clean_method_source to remove comments.
Feb 2, 2019
bafc25d
Updated clean_method_source, remove duplicate and trailing whitespaces.
Feb 2, 2019
172115a
Included test coverage for variable with "return" on the name.
Feb 2, 2019
d5e0de8
Updated regex with word bondaries.
Feb 2, 2019
0e61d2c
Shortened line.
Feb 2, 2019
4c89beb
Merged return_not_documented tests into simplified version.
Feb 3, 2019
b04110a
Removed random elements from tests.
dluiscosta Feb 5, 2019
4b01092
Switched to AST approach.
dluiscosta Feb 5, 2019
0928ba6
Removed clean_method_source property.
dluiscosta Feb 5, 2019
57fdb1e
Updated does_nothing test not to leave variable unused.
dluiscosta Feb 5, 2019
00d5fdb
Reestructured and minimized tests.
dluiscosta Feb 7, 2019
0a0c05e
Encapsulated search for returns on method_returns property.
dluiscosta Feb 7, 2019
6ab2534
Updated method_returns property to ignore nested functions.
dluiscosta Feb 7, 2019
504ea10
Updated ast.parse call.
dluiscosta Feb 7, 2019
17737d1
Fixed linting errors.
dluiscosta Feb 7, 2019
fe23351
Updated method_source to remove common indentation from lines.
dluiscosta Feb 7, 2019
783eb39
Corrected method_source.
dluiscosta Feb 7, 2019
2c41bd7
Reverted ast.parse call.
dluiscosta Feb 7, 2019
6e53215
Updated method_returns to respect empty method_sources.
dluiscosta Feb 8, 2019
2c60e4d
Fixed linting error.
dluiscosta Feb 8, 2019
11b827b
Merge branch 'master' into check-return-necessity
dluiscosta Feb 9, 2019
3e93cbe
Updated method_source property, removing from try block what can't fail.
dluiscosta Feb 13, 2019
1660060
Refactored method_returns to method_returns_something.
dluiscosta Feb 13, 2019
be6c906
Added docstring to method_returns_something.
dluiscosta Feb 13, 2019
0b361de
Improved method_returns_something readability.
dluiscosta Feb 13, 2019
2ebfa33
Corrected method_returns_something property docstring.
dluiscosta Feb 13, 2019
65b0f56
Removed unecessary print from test no_returns.
datapythonista Feb 13, 2019
bbf39d0
Simplified return_not_documented test.
dluiscosta Feb 13, 2019
2c66c89
Removed mistakenly added whitespaces.
dluiscosta Feb 13, 2019
d99edb3
Merge branch 'master' into check-return-necessity
dluiscosta Feb 13, 2019
07a4b01
Added dedent.
dluiscosta Feb 14, 2019
4d6ec7e
Merge branch 'master' into check-return-necessity
dluiscosta Feb 19, 2019
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
28 changes: 26 additions & 2 deletions scripts/tests/test_validate_docstrings.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,27 @@ def good_imports(self):
"""
pass

def no_returns(self):
"""
Say hello and have no returns.
"""
print("Hello World!")
dluiscosta marked this conversation as resolved.
Show resolved Hide resolved

def empty_returns(self):
"""
Say hello and always return None.

Since this function never returns a value, this
docstring doesn't need a return section.
"""
def say_hello():
return "Hello World!"
say_hello()
if True:
return
else:
return None
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test for a property that returns something (without the Returns section, which is the correct).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

such a test was already present before this PR:

    def return_not_documented(self):
        """
        Lacks section for Returns
        """
        return "Hello world!"

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 is negative test, not like the one being commented here, so it's inside the BadReturns class



class BadGenericDocStrings(object):
"""Everything here has a bad docstring
Expand Down Expand Up @@ -594,7 +615,10 @@ def return_not_documented(self):
"""
Lacks section for Returns
"""
return "Hello world!"
if False:
return None
else:
return True

def yield_not_documented(self):
"""
Expand Down Expand Up @@ -783,7 +807,7 @@ def test_good_class(self, capsys):

@pytest.mark.parametrize("func", [
'plot', 'sample', 'random_letters', 'sample_values', 'head', 'head1',
'contains', 'mode', 'good_imports'])
'contains', 'mode', 'good_imports', 'no_returns', 'empty_returns'])
def test_good_functions(self, capsys, func):
errors = validate_one(self._import_path(
klass='GoodDocStrings', func=func))['errors']
Expand Down
36 changes: 34 additions & 2 deletions scripts/validate_docstrings.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import importlib
import doctest
import tempfile
import ast
TomAugspurger marked this conversation as resolved.
Show resolved Hide resolved

import flake8.main.application

Expand Down Expand Up @@ -490,10 +491,41 @@ def yields(self):
@property
def method_source(self):
try:
return inspect.getsource(self.obj)
source = inspect.getsource(self.obj)
dluiscosta marked this conversation as resolved.
Show resolved Hide resolved
# Remove common indentation.
first_spaces = re.match(' +', source)
if first_spaces:
first_spaces = first_spaces.group(0)
source = re.sub('^' + first_spaces, '', source)
source = re.sub(r'\n' + first_spaces, r'\n', source)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is happening here is that, when inspect.getsource() runs, it returns the function's code as it is on the source file, indentation included. So, for example, if the function is a method:

class A:
    def B(self):
        return None

Getting the source of B will return

    def B(self):
        return None

with the previously present indentation kept.

If we feed this directly to ast.parse(), it causes an indentation error, since the first thing read on the string is a space, and it is unexpected.

An easy fix is to apply .strip() to the string containing the function source, which remove all whitespaces in the beginning and the end. In my example, the result would be:

def B(self):
        return None

Which solves the previous problem.

The thing is, on more complex cases, this approach isn't enough. Consider the real example:

    @Appender(_shared_docs['melt'] %
              dict(caller='df.melt(',
                   versionadded='.. versionadded:: 0.20.0\n',
                   other='melt'))
    def melt(self, id_vars=None, value_vars=None, var_name=None,
             value_name='value', col_level=None):
        from pandas.core.reshape.melt import melt
        return melt(self, id_vars=id_vars, value_vars=value_vars,
                    var_name=var_name, value_name=value_name,
                    col_level=col_level)

By applying .strip(), we get:

@Appender(_shared_docs['melt'] %
              dict(caller='df.melt(',
                   versionadded='.. versionadded:: 0.20.0\n',
                   other='melt'))
    def melt(self, id_vars=None, value_vars=None, var_name=None,
             value_name='value', col_level=None):
        from pandas.core.reshape.melt import melt
        return melt(self, id_vars=id_vars, value_vars=value_vars,
                    var_name=var_name, value_name=value_name,
                    col_level=col_level)

This leaves the @Appender (I think it's a decorator) 1 indentation level bellow the function's signature, which is not proper syntax and raises another indentation error.

To avoid this problem, the present solution removes the same number os whitespaces from each line, as if the function was declared at an otherwise empty file:

@Appender(_shared_docs['melt'] %
          dict(caller='df.melt(',
               versionadded='.. versionadded:: 0.20.0\n',
               other='melt'))
def melt(self, id_vars=None, value_vars=None, var_name=None,
         value_name='value', col_level=None):
    from pandas.core.reshape.melt import melt
    return melt(self, id_vars=id_vars, value_vars=value_vars,
                var_name=var_name, value_name=value_name,
                col_level=col_level)

That said, I'm open to suggestions if someone can think of a simpler way to solve the described problem.

return source
except TypeError:
return ''

@property
def method_returns(self):
dluiscosta marked this conversation as resolved.
Show resolved Hide resolved
tree = ast.parse(self.method_source).body
if tree:
root = tree[0]

def gather_returns(node):
Copy link
Member

Choose a reason for hiding this comment

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

Hmm is there any way to simplify what is going on here? Is there no way to extract the .returns from the AST directly instead of having to recurse everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our case, there isn't.

The AST provides the .walk() method to iterate through nodes. According to the doc, it

is useful if you only want to modify nodes in place and don’t care about the context.

Without context, there is no way to tell if a return node is inside a nested inner function or not, which is needed for one of the requirements raised by @datapythonista.

I explain this a bit further here.

dluiscosta marked this conversation as resolved.
Show resolved Hide resolved
gathered = [node] if isinstance(node, ast.Return) else []
for child in ast.iter_child_nodes(node):
# Ignore nested functions and its subtrees.
if not isinstance(child, ast.FunctionDef):
gathered.extend(gather_returns(child))
return gathered
# Walk the tree recursively and gather the return nodes.
return_values = [r.value for r in gather_returns(root)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As opposing to using ast.walk() to iterate over all the nodes on the method's AST, here I'm recursively walking the tree myself.

The reason for that is to respect cases like the following:

def A():
    def B():
        return "Hello World!"
    print(B())
    return None

In this case, there is a inner function which returns a string, but the function being evaluated itself always return None, thus shouldn't have a Returns section on the docstring.

If I read all the nodes indiscriminately, as happens with ast.walk(), I can't distinguish returns inside nested functions from returns on the function actually being evaluated. If I search trough it myself, though, I can prune the recursion when I reach a (nested) function node, thus ignoring it's entire subtree and possible return nodes in it.


# Replace NameConstant nodes valued None for None.
for i, v in enumerate(return_values):
if isinstance(v, ast.NameConstant) and v.value is None:
return_values[i] = None
return return_values
else:
return []

@property
def first_line_ends_in_dot(self):
if self.doc:
Expand Down Expand Up @@ -691,7 +723,7 @@ def get_validation_data(doc):

if doc.is_function_or_method:
if not doc.returns:
if 'return' in doc.method_source:
if any(ret is not None for ret in doc.method_returns):
errs.append(error('RT01'))
else:
if len(doc.returns) == 1 and doc.returns[0][1]:
Expand Down