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

dluiscosta · 2019-01-29T18:23:09Z

closes DOC: Detect whether a Returns section is needed in validate_docstrings.py #23488

Updated return lookup at source in validate_docstrings.py:

ignore "return None"
ignore empty return
ignore the word "return" on commentaries

Updated test_validate_docstrings.py:

added a test which contains the "returns" listed above and has a valid docstring with no Return section

…ommentary

pep8speaks · 2019-01-29T18:23:13Z

Hello @dluiscosta! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 19, 2019 at 06:04 Hours UTC

dluiscosta · 2019-01-29T18:37:47Z

Added another "return" occurrence to the docstring in the test, on the beginning of a line.
It broke my solution.

codecov · 2019-01-29T19:45:18Z

Codecov Report

Merging #25008 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25008      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52401    52401              
==========================================
+ Hits        48409    48410       +1     
+ Misses       3992     3991       -1

Flag	Coverage Δ
#multiple	`90.8% <ø> (ø)`	⬆️
#single	`42.88% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/testing.py	`88.13% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abf0824...d35ec84. Read the comment docs.

codecov · 2019-01-29T19:45:21Z

Codecov Report

Merging #25008 into master will decrease coverage by 3.97%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #25008      +/-   ##
==========================================
- Coverage   91.73%   87.76%   -3.98%     
==========================================
  Files         173      173              
  Lines       52839    57856    +5017     
==========================================
+ Hits        48472    50775    +2303     
- Misses       4367     7081    +2714

Flag	Coverage Δ
#multiple	`90.31% <ø> (+0.01%)`	⬆️
#single	`43% <ø> (+1.28%)`	⬆️

Impacted Files	Coverage Δ
pandas/io/excel/_util.py	`56.83% <0%> (-23.82%)`	⬇️
pandas/core/indexes/multi.py	`75.43% <0%> (-20.18%)`	⬇️
pandas/core/frame.py	`78.29% <0%> (-18.59%)`	⬇️
pandas/core/indexes/datetimes.py	`78.31% <0%> (-17.99%)`	⬇️
pandas/core/indexes/category.py	`81.52% <0%> (-17.1%)`	⬇️
pandas/core/generic.py	`77.88% <0%> (-16.29%)`	⬇️
pandas/core/arrays/numpy_.py	`78.68% <0%> (-14.83%)`	⬇️
pandas/core/arrays/timedeltas.py	`74.56% <0%> (-13.74%)`	⬇️
pandas/io/excel/_base.py	`78.88% <0%> (-13.64%)`	⬇️
pandas/core/indexes/base.py	`85.1% <0%> (-11.46%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f74aba6...4d6ec7e. Read the comment docs.

scripts/validate_docstrings.py

scripts/tests/test_validate_docstrings.py

scripts/validate_docstrings.py

datapythonista

I think we should use a more reliable approach, see comments.

scripts/tests/test_validate_docstrings.py

scripts/validate_docstrings.py

dluiscosta · 2019-02-08T07:45:22Z

scripts/validate_docstrings.py

+                        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)]


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.

dluiscosta · 2019-02-08T08:03:11Z

I'd appreciate some help understanding the reason for the current Travis fail. Other than that, I have made the suggested changes and apparently fixed the previous problems, @datapythonista, @WillAyd.

WillAyd · 2019-02-09T01:56:12Z

Travis failure is unrelated to your change and was resolved in #25225 . If you can merge master and re push should go away

Merge updated master to solve Travis issue.

dluiscosta · 2019-02-09T12:05:13Z

Thanks @WillAyd, solved it.

WillAyd · 2019-02-12T05:27:10Z

scripts/validate_docstrings.py

+        if tree:
+            root = tree[0]
+
+            def gather_returns(node):


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?

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.

datapythonista

looks good, just few comments

scripts/validate_docstrings.py

datapythonista · 2019-02-13T17:03:32Z

scripts/tests/test_validate_docstrings.py

+        if True:
+            return
+        else:
+            return None


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

such a test was already present before this PR:

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

it is negative test, not like the one being commented here, so it's inside the BadReturns class

scripts/tests/test_validate_docstrings.py

Co-Authored-By: dluiscosta <dluiscosta@gmail.com>

scripts/validate_docstrings.py

dluiscosta · 2019-02-14T12:26:00Z

I don't understand why the tests are failing. Locally, everything is fine.

I tried once again to merge master, which had solved a previous problem, but this time it didn't help.

WillAyd · 2019-02-19T04:05:08Z

@dluiscosta can you merge master? Should resolve Py27 failure

dluiscosta · 2019-03-03T00:31:47Z

Hey @datapythonista, @WillAyd, is there something left to be done here?

WillAyd

OK by me - @datapythonista

jreback · 2019-03-10T23:17:13Z

ping @datapythonista @jorisvandenbossche @TomAugspurger

TomAugspurger · 2019-03-11T12:01:54Z

Thanks @dluiscosta!

* upstream/master: (110 commits) DOC: hardcode contributors for 0.24.x releases (pandas-dev#25662) DOC: restore toctree maxdepth (pandas-dev#25134) BUG: Redefine IndexOpsMixin.size, fix pandas-dev#25580. (pandas-dev#25584) BUG: to_csv line endings with compression (pandas-dev#25625) DOC: file obj for to_csv must be newline='' (pandas-dev#25624) Suppress incorrect warning in nargsort for timezone-aware DatetimeIndex (pandas-dev#25629) TST: fix incorrect sparse test (now failing on scipy master) (pandas-dev#25653) CLN: Removed debugging code (pandas-dev#25647) DOC: require Return section only if return is not None nor commentary (pandas-dev#25008) DOC:Remove hard-coded examples from _flex_doc_SERIES (pandas-dev#24589) (pandas-dev#25524) TST: xref pandas-dev#25630 (pandas-dev#25643) BUG: Fix pandas-dev#25481 by fixing the error message in TypeError (pandas-dev#25540) Fixturize tests/frame/test_mutate_columns.py (pandas-dev#25642) Fixturize tests/frame/test_join.py (pandas-dev#25639) Fixturize tests/frame/test_combine_concat.py (pandas-dev#25634) Fixturize tests/frame/test_asof.py (pandas-dev#25628) BUG: Fix user-facing AssertionError with to_html (pandas-dev#25608) (pandas-dev#25620) DOC: resolve all GL03 docstring validation errors (pandas-dev#25525) TST: failing wheel building on PY2 and old numpy (pandas-dev#25631) DOC: Remove makePanel from docs (pandas-dev#25609) (pandas-dev#25612) ...

Return section only required if at least one return is not None nor c…

575abc4

…ommentary

Daniel Costa added 2 commits January 29, 2019 16:24

fixed PEP8 issues

4b6406f

added another "return" on test

b204f11

Daniel Costa added 2 commits January 29, 2019 16:54

now respects summary in a single line

0a76a3a

only look for return commands after docstring

d35ec84

jschendel added Docs CI Continuous Integration labels Jan 29, 2019

WillAyd requested changes Jan 30, 2019

View reviewed changes

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

Daniel Costa added 2 commits January 30, 2019 15:53

Added comments to regex that finds non-bare return commands

e7d9bed

added space around bitwise or operator

2cdd367

jbrockmendel reviewed Jan 31, 2019

View reviewed changes

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

Replaced [ \t\f\v] for \s on regex.

0e25d93

WillAyd requested changes Jan 31, 2019

View reviewed changes

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

Daniel Costa added 10 commits February 1, 2019 00:14

Added test with valued return after bare returns

d5d9800

method_source now returns source without docstring

073a6b6

Updated to search string without whitespaces.

0143b7c

Simplified regex.

76188dd

Created clean_method_source property without comments or docstring.

2051ea6

Updated clean_method_source to remove comments.

ff17a79

Updated clean_method_source, remove duplicate and trailing whitespaces.

bafc25d

Included test coverage for variable with "return" on the name.

172115a

Updated regex with word bondaries.

d5e0de8

Shortened line.

0e61d2c

WillAyd requested changes Feb 3, 2019

View reviewed changes

scripts/tests/test_validate_docstrings.py Outdated Show resolved Hide resolved

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

Merged return_not_documented tests into simplified version.

4c89beb

datapythonista requested changes Feb 5, 2019

View reviewed changes

scripts/tests/test_validate_docstrings.py Outdated Show resolved Hide resolved

scripts/tests/test_validate_docstrings.py Outdated Show resolved Hide resolved

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

dluiscosta commented Feb 8, 2019

View reviewed changes

Merge branch 'master' into check-return-necessity

11b827b

Merge updated master to solve Travis issue.

WillAyd requested changes Feb 12, 2019

View reviewed changes

datapythonista reviewed Feb 13, 2019

View reviewed changes

dluiscosta and others added 9 commits February 13, 2019 15:30

Updated method_source property, removing from try block what can't fail.

3e93cbe

Refactored method_returns to method_returns_something.

1660060

Added docstring to method_returns_something.

be6c906

Improved method_returns_something readability.

0b361de

Corrected method_returns_something property docstring.

2ebfa33

Removed unecessary print from test no_returns.

65b0f56

Co-Authored-By: dluiscosta <dluiscosta@gmail.com>

Simplified return_not_documented test.

bbf39d0

Removed mistakenly added whitespaces.

2c66c89

Merge branch 'master' into check-return-necessity

d99edb3

WillAyd requested changes Feb 14, 2019

View reviewed changes

scripts/validate_docstrings.py Outdated Show resolved Hide resolved

scripts/validate_docstrings.py Show resolved Hide resolved

Added dedent.

07a4b01

Merge branch 'master' into check-return-necessity

4d6ec7e

WillAyd approved these changes Mar 6, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone Mar 10, 2019

TomAugspurger approved these changes Mar 11, 2019

View reviewed changes

TomAugspurger merged commit e2d0ad0 into pandas-dev:master Mar 11, 2019

WillAyd mentioned this pull request Apr 29, 2019

DOC: Fix validation error type RT01 and check in CI (#25356) #26234

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dluiscosta commented Jan 29, 2019 •

edited

Loading

pep8speaks commented Jan 29, 2019 •

edited

Loading

dluiscosta commented Jan 29, 2019

codecov bot commented Jan 29, 2019

codecov bot commented Jan 29, 2019 •

edited

Loading

datapythonista left a comment

dluiscosta Feb 8, 2019

dluiscosta commented Feb 8, 2019

WillAyd commented Feb 9, 2019

dluiscosta commented Feb 9, 2019

WillAyd Feb 12, 2019

dluiscosta Feb 13, 2019

datapythonista left a comment

datapythonista Feb 13, 2019

dluiscosta Feb 13, 2019

dluiscosta Feb 13, 2019

dluiscosta commented Feb 14, 2019

WillAyd commented Feb 19, 2019

dluiscosta commented Mar 3, 2019

WillAyd left a comment

jreback commented Mar 10, 2019

TomAugspurger commented Mar 11, 2019

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

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

Conversation

dluiscosta commented Jan 29, 2019 • edited Loading

pep8speaks commented Jan 29, 2019 • edited Loading

Comment last updated on February 19, 2019 at 06:04 Hours UTC

dluiscosta commented Jan 29, 2019

codecov bot commented Jan 29, 2019

Codecov Report

codecov bot commented Jan 29, 2019 • edited Loading

Codecov Report

datapythonista left a comment

Choose a reason for hiding this comment

dluiscosta Feb 8, 2019

Choose a reason for hiding this comment

dluiscosta commented Feb 8, 2019

WillAyd commented Feb 9, 2019

dluiscosta commented Feb 9, 2019

WillAyd Feb 12, 2019

Choose a reason for hiding this comment

dluiscosta Feb 13, 2019

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista Feb 13, 2019

Choose a reason for hiding this comment

dluiscosta Feb 13, 2019

Choose a reason for hiding this comment

dluiscosta Feb 13, 2019

Choose a reason for hiding this comment

dluiscosta commented Feb 14, 2019

WillAyd commented Feb 19, 2019

dluiscosta commented Mar 3, 2019

WillAyd left a comment

Choose a reason for hiding this comment

jreback commented Mar 10, 2019

TomAugspurger commented Mar 11, 2019

dluiscosta commented Jan 29, 2019 •

edited

Loading

pep8speaks commented Jan 29, 2019 •

edited

Loading

codecov bot commented Jan 29, 2019 •

edited

Loading