Fix Python docstring type handling bug

[ProGamerGov]

This PR ensures that NumPy style docstrings using 'of' are properly handled.

For example:

def func(t, x, y):
    """
    Args:

        t (int or float): Variable description.
        x (list of list of int, optional): Variable description.
        y (tensor or tuple of tensors): Variable description.
    """ 

According to the regex used here: https://github.com/sphinx-doc/sphinx/blob/master/sphinx/domains/python.py#L381, only some of the variables in the above function's docstring will be properly handled. Anything with 'of' in it will not be properly separated by the regex code. This PR fixes this issue.

If this PR is on the wrong branch, let me know!

Feature or Bugfix

• Bugfix

Purpose

• Fixes docstring format handling bug.

Detail

Below are some examples of failures made visible by using Intersphinx. Notably types using 'of' are not properly hyperlinked.

Relates

• N/A

[AA-Turner]

Is the of delimiter endorsed by the google style guide?

A

[ProGamerGov]

@AA-Turner Yeah, you can see an example here: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

        Args:
            param1 (str): Description of `param1`.
            param2 (:obj:`int`, optional): Description of `param2`. Multiple
                lines are supported.
            param3 (:obj:`list` of :obj:`str`): Description of `param3`.

I've noticed the issue in projects like pytorch/captum.

[AA-Turner]

The link you provided is from the now-archived sphinxcontrib.napolean project. Is there an example from the official google style guide rather than something third-party?

A

[AA-Turner]

https://www.sphinx-doc.org/en/master/usage/extensions/example_google.html seems to be the up-to-date copy.

[ProGamerGov]

@AA-Turner So, actually I may have confused Google's style guide with NumPy's style guide. NumPy's official style guidelines show list of int being used here: https://numpydoc.readthedocs.io/en/latest/format.html#parameters

Google's style guide though doesn't seem to explicitly mention it: https://google.github.io/styleguide/pyguide.html

Edit: I see you found a google style guide that shows list(int)

[AA-Turner]

NumpyDocstring is a subclass of GoogleDocstring. Please may you update the PR to override the regular expression only in NumpyDocstring, and add a link back to the style guide with an explanation as to why we are overriding the regular expression.

I entirely forgot that this PR doesn't touch sphinx.ext.napolean. Perhaps we just leave the PR as-is and accept the current change?

A

[ProGamerGov]

@AA-Turner Yeah, we can do that. Though I'm not 100% sure of the regex works as intended. My usage of the or operator slightly removed the spaces around or in the output.

Instead of outputting this:

['\n    Args:\n\n        t', ' (', 'int', ' or ', 'float', ')', ': Variable description.\n        x', ' (', 'list of list of int', ', ', 'optional', ')', ': Variable description.\n        y', ' (', 'tensor', ' or ', 'tuple of tensors', ')', ': Variable description.\n    ']

It now outputs this:

['\n    Args:\n\n        t', ' (', 'int', ' or', ' float', ')', ': Variable description.\n        x', ' (', 'list ', 'of ', 'list ', 'of ', 'int', ', ', 'optional', ')', ': Variable description.\n        y', ' (', 'tensor', ' or', ' tuple ', 'of ', 'tensors', ')', ': Variable description.\n    ']

It's a subtle difference of ' or' instead of ' or '

[ProGamerGov]

@AA-Turner We can merge the PR now in its current form as it resolves the issue!

Example:

The sphinx.ext.napolean extension doesn't seem to require any changes to fully support this change.

[AA-Turner]

Please add tests ensuring the behaviour doesn't regress in the future.

A

[AA-Turner]

I have a slight concern over this change in that it encourages non-standard descriptions of typing -- for example if I had an object that was a list[tuple[date, ...]], I think using that format in the docstring is much better than list of tuple of int.

I don't know if napoleon supports the type-annotation format though, and cannot test at the moment (on mobile).

A

[ProGamerGov]

@AA-Turner Do you happen to know which testing file I should add the tests to?

And yeah, that could be a better format. Though using of is still somewhat common in many projects.

Also, as far as I'm aware, napoleon handles list[tuple[date, ...]] and a quick test verifies this:

var1 (list[tuple[date, ...]]): Variable description.

[AA-Turner]

See tests/test_ext_napoleon_docstring.py, around line 160 for an exemplar.

Thanks for confirming the 'proper' style works. Please also add a CHANGES entry and add yourself to AUTHORS.

A

[ProGamerGov]

@AA-Turner Thanks! I've added myself to the AUTHORS file and added a new test for the changes. For the CHANGES file, I wasn't 100% sure what to say for the change, so let me know if I need to change it or anything else

[ProGamerGov]

So, the tests don't currently seem to fail if the changes in this PR haven't been introduced. I think that part of the existing code may have already been capable of handing cases with 'of', and thus the tests will always pass.

[ProGamerGov]

@AA-Turner Testing now verifies that functionality introduced in this PR remains present. As the changes in this PR were on a single line in sphinx/domains/python.py, I added tests to tests/test_domain_py.py. These tests actually do fail if someone breaks the regex code changes introduced in this PR. I also added another test for the or deliminator as I couldn't find any examples of it being tested.

I think that this PR is finally ready for merging!

[AA-Turner]

Looks good otherwise, please could you also look at the flake8 failure and then ping me.

A

[ProGamerGov]

@AA-Turner I've fixed the formatting for flake8

[AA-Turner]

Thanks Ben!

A