require Return section only if return is not None nor commentary
State: closed

- [ ] closes #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

Labels:
name: Docs
name: CI
description: Continuous Integration

Comments:
Hello @dluiscosta! Thanks for updating the PR.





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

##### Comment last updated on February 19, 2019 at 06:04 Hours UTC
Added another "return" occurrence to the docstring in the test, on the beginning of a line.
It broke my solution.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=h1) Report
> Merging [#25008](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/abf0824200a34c00f6a20b283bbb968f2c2d288f?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25008/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=tree)

```diff
@@            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% <ø> (ø)` | :arrow_up: |
| #single | `42.88% <ø> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.13% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=footer). Last update [abf0824...d35ec84](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=h1) Report
> Merging [#25008](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f74aba614554d1de4d7986fef6296a4eda951ac9?src=pr&el=desc) will **decrease** coverage by `3.97%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25008/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=tree)

```diff
@@            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%)` | :arrow_up: |
| #single | `43% <ø> (+1.28%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/excel/\_util.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL191dGlsLnB5) | `56.83% <0%> (-23.82%)` | :arrow_down: |
| [pandas/core/indexes/multi.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9tdWx0aS5weQ==) | `75.43% <0%> (-20.18%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `78.29% <0%> (-18.59%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `78.31% <0%> (-17.99%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `81.52% <0%> (-17.1%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `77.88% <0%> (-16.29%)` | :arrow_down: |
| [pandas/core/arrays/numpy\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL251bXB5Xy5weQ==) | `78.68% <0%> (-14.83%)` | :arrow_down: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `74.56% <0%> (-13.74%)` | :arrow_down: |
| [pandas/io/excel/\_base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2V4Y2VsL19iYXNlLnB5) | `78.88% <0%> (-13.64%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `85.1% <0%> (-11.46%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/pandas-dev/pandas/pull/25008/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=footer). Last update [f74aba6...4d6ec7e](https://codecov.io/gh/pandas-dev/pandas/pull/25008?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@datapythonista, @WillAyd, I have switched from using regex to a approach based on AST.

Pros:
-  It requires no additional properties for preprocessing the method source.
-  It's probably more reliable.

Con: It looks a little complex to me. Tell me if you think any comments are called for.
There appears to be a new issue caught by the automated tests.

It's an unexpected indentation when building the AST tree. I had the same error locally when developing the last commits: the method_source gets the code with whatever indentation it has on pandas source. If I send the method_source directly to AST, the first few spaces, before the "def" keyword, are unexpected and thus raise an error. Using .split() to remove spaces on the beginning of the source was enough to solve this.

On the other hand, with the tests run at Github, there is at least one case for which that solution is not enough. I'm not sure, but looks like the error was raised in the line of a decorator.

Is there a way to run those tests locally so I can try to investigate the problem further? Anyone can offer me some thoughts on this?


`python -m pytest pandas` should run the tests (there are different tests sets, so it's a bit more complex than that, but I think it should be enough with this command)
Thanks, I think I understand. I was running the tests over the `validate_docstring.py`, but once I pulled, all the tests on pandas were run and the `validate_docstring.py` is itself part of other tests.

I'll use that to try to discover where the error is happening and attempt to reproduce it in a new `test_validate_docstring.py` test, as soon as I get the chance.
Sorry, if the tests you were referring to were the tests of the validation script, then it's `python -m pytest scripts/`. I'm reviewing few PRs, and it's difficult to get the context when changing from one to another, sorry about that.

Regarding the tests, every developer probably has a different opinion, but the most important principles for me are:
- Keeping the complexity as low as possible, and even more in a huge and complex project like pandas
- Test all the reasonable cases that can prevent problems

In this case, that would mean minimal tests for cases like (not sure if I'm missing anything):
- No return statement
- Only `return`/`return None` statements
- Return from a constant
- Return from a variable
- Yield
- A return in a subfunction that should be ignored, for example:
```
def foo():
    def bar():
        return True
    return None
```
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.
Travis failure is unrelated to your change and was resolved in #25225 . If you can merge master and re push should go away
Thanks @WillAyd, solved it.
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.
@dluiscosta can you merge master? Should resolve Py27 failure
Hey @datapythonista, @WillAyd, is there something left to be done here?
ping @datapythonista @jorisvandenbossche @TomAugspurger 
Thanks @dluiscosta!