Skip to content

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

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

Closed
datapythonista opened this issue Nov 4, 2018 · 7 comments · Fixed by #25008
Closed

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

datapythonista opened this issue Nov 4, 2018 · 7 comments · Fixed by #25008
Labels
CI Continuous Integration Docs good first issue
Milestone

Comments

@datapythonista
Copy link
Member

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:

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.

@WillAyd
Copy link
Member

WillAyd commented Nov 4, 2018

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

@TomAugspurger
Copy link
Contributor

I had hoped that the ast module would work out of the box, but not quite.

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.

@datapythonista
Copy link
Member Author

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. :)

@dluiscosta
Copy link
Contributor

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.

@datapythonista
Copy link
Member Author

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.

@dluiscosta
Copy link
Contributor

Thank you for the help, I was trying to run the test_validation_docstrings.py directly. Now it's working.

@dluiscosta
Copy link
Contributor

@datapythonista , I've created a pull request. Can you please take a look?

Being that my first one, I'd appreciate any sort of feedback.

@jreback jreback added this to the 0.25.0 milestone Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Docs good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants