Repository: pandas-dev/pandas
Pull request: require Return section only if return is not None nor commentary
- [ ] 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

Problem statement:
DOC: Detect whether a Returns section is needed in validate_docstrings.py
One of the validations performed in the docstrings in `validate_docstrings.py` is whether a `Returns` section is present. So far we just check if the `return` word exist in the source code, and if it does, we generate an error if the docsting does not have the documentation section.

But this causes false positives, see this case:
```python
def say_hello(lang):
    """
    This function does not have a Returns section.

    This is correct because nothing is returned.

    Parameters
    ----------
    lang : str
         Language to say hello in.
     """
     if lang not in ('en', 'es'):
         return

    print({'en': 'Hi!', 'es': 'Hola!'}[lang])
```

The `return` keyword is found in the code, so the presence of a `Returns` section is validated, but it should not.

Without making anything complicated, it'd be nice to check when validating:
- That the return words found are not a bare return or a `return None` or are not comments...

If any code `return` is found that does not return always `None` is found, then we should show the error.

Hints from issue discussion:
Moving our conversation here from the referenced issue. Understood now what you are saying.

Unless we have some kind of code that performs a conditional mutation on an argument would this ever really show up? If not just not sure it's worth the extra complexity
I had hoped that the `ast` module would work out of the box, but not quite.


```python
In [44]: tree = ast.parse(inspect.getsource(pd.DataFrame.head).strip())

In [45]: tree.body[0].returns

```

https://stackoverflow.com/a/48366253/1889400 has some suggestions.

So mostly in agreement with @WillAyd, maybe not worth the trouble.
I think with a regex should be quite straight forward.

For the short term I don't think it's important. But as we manage to add the `validation_docstrings.py` in the CI, if we can't identify if the function can return something other than None, we won't be able to validate docstrings with missing Returns section.

But we can come back to it once we're there. So much to do before. :)
Hi.

This will be my first attempt at contributing to open source. Can someone help me out?

So far I have written a regex that I believe solves the problem at hand. I tested it in a separated file, against a few cases I could think of, since I can't really figure out how to use test_validation_docstrings.py. Is there an automated test I can run, and how?

I have also written a method to the GoodDocStrings class that has:

- an empty return
- a "return None"
- a commentary with the word "return"

but has no Return section on the docstring.

Can I place those codes here? Or should I open a pull request even without having run the test_validation_docstrings.py myself? Please, I need some orientation.



What's the problem with `test_validation_docstrings.py`? Can't you run the tests? In a pandas environment, you should simply run in the root `python -m pytest scripts/`.

If that was the problem, feel free to open a separate PR to clarify that in the documentation for pandas developers.

When you're happy with your changes, open a PR, posting the code in a comment is not really useful.
Thank you for the help, I was trying to run the test_validation_docstrings.py directly. Now it's working.

Commit message: added space around bitwise or operator

Patch to review:
diff --git a/scripts/tests/test_validate_docstrings.py b/scripts/tests/test_validate_docstrings.py
index bb58449843096..615c81b8b9ec2 100644
--- a/scripts/tests/test_validate_docstrings.py
+++ b/scripts/tests/test_validate_docstrings.py
@@ -229,6 +229,23 @@ def good_imports(self):
         """
         pass
 
+    def does_nothing(self, x):
+        """
+        Do nothing and always return none.
+
+        But in other cases it might also
+        return nothing at all.
+
+        Parameters
+        ----------
+        x : int
+            Useless parameter.
+        """
+        if random.random() % 2:
+            return  # the method might return here
+        else:
+            return None  # or perhaps here
+
 
 class BadGenericDocStrings(object):
     """Everything here has a bad docstring
@@ -783,7 +800,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', 'does_nothing'])
     def test_good_functions(self, capsys, func):
         errors = validate_one(self._import_path(
             klass='GoodDocStrings', func=func))['errors']
diff --git a/scripts/validate_docstrings.py b/scripts/validate_docstrings.py
index bce33f7e78daa..47211d60ad805 100755
--- a/scripts/validate_docstrings.py
+++ b/scripts/validate_docstrings.py
@@ -691,7 +691,11 @@ def get_validation_data(doc):
 
     if doc.is_function_or_method:
         if not doc.returns:
-            if 'return' in doc.method_source:
+            if re.search(r""" \"\"\".*\"\"\".*  # Skip the docstring.
+                              \n[ \t\f\v]*return  # Find a return command.
+                              # Check if it's not bare or simply returns None.
+                              (?![ \t\f\v]*(None)?[ \t\f\v]*[\n#]) """,
+                         doc.method_source, re.DOTALL | re.VERBOSE):
                 errs.append(error('RT01'))
         else:
             if len(doc.returns) == 1 and doc.returns[0][1]:


Code review comments:
diff_hunk: @@ -691,7 +691,11 @@ def get_validation_data(doc):
 
     if doc.is_function_or_method:
         if not doc.returns:
-            if 'return' in doc.method_source:
+            if re.search(r""" \"\"\".*\"\"\".*  # Skip the docstring.
+                              \n[ \t\f\v]*return  # Find a return command.
path: scripts/validate_docstrings.py
text: @user1:
Why these specific whitespace characters and not just  \s?

@author:
At first, I used `\s`, but was receiving this deprecation warning:

```
scripts/validate_docstrings.py:695
  /Users/danielcosta/pandas-daniel/scripts/validate_docstrings.py:695: DeprecationWarning: invalid escape sequence \s
    '\n\s*return' +
```

[re documentation](https://docs.python.org/3/library/re.html) shows that `\s` is equivalente to `[ \t\n\r\f\v]`, so I just removed the end line and carriage return (`\n` and `\r`), which are not what I intend to consume there, and left the others. Sincerely, I couldn't find out the meaning of `\f` and `\v`.

That said, I just tested using `\s` again and it seems that while using `re.VERBOSE` I don't get any more warnings. That leaves me wondering if using `\s` in regex is really discouraged by some reason or if it was just a wrongful warning that I was receiving. Maybe you can help me clarifying this.

I'll hold on with making the substitution until then.

@author:
I found out that the deprecation warning didn't go away with the `re.VERBOSE`, but with the usage of a raw string, and now I understand why it was happening in the first place.

Thus I replaced `[ \t\f\v]` for `\s`.