Sphinx: Warning: Inline literal start-string without end-string in Numpy style Parameters section

[MatthieuDartiailh]

Describe the bug
The following docstring generates a warning on the line of the timeout parameter. Removing the quote around default cause the warning to go away.

def lock(
        self,
        timeout: Union[float, Literal["default"]] = "default",
        requested_key: Optional[str] = None,
    ) -> str:
        """Establish a shared lock to the resource.

        Parameters
        ----------
        timeout : Union[float, Literal["default"]], optional
            Absolute time period (in milliseconds) that a resource waits to get
            unlocked by the locking session before returning an error.
            Defaults to "default" which means use self.timeout.
        requested_key : Optional[str], optional
            Access key used by another session with which you want your session
            to share a lock or None to generate a new shared access key.

        Returns
        -------
        str
            A new shared access key if requested_key is None, otherwise, same
            value as the requested_key

        """

To Reproduce
Steps to reproduce the behavior:

$ git clone https://github.com/pyvisa/pyvisa
$ git checkout pytest
$ cd pyvisa
$ pip install -e .
$ cd docs
$ sphinx-build source build -W -b html;

Expected behavior
I do not expect to see a warning there and was not seeing any before 3.2

Your project
The project is build under the Documentation build action. https://github.com/pyvisa/pyvisa/pull/531

Environment info

• OS: Mac Os and Linux
• Python version: 3.8.2 and 3.8.5
• Sphinx version: 3.2.0
• Sphinx extensions: "sphinx.ext.autodoc", "sphinx.ext.doctest","sphinx.ext.intersphinx", "sphinx.ext.coverage", "sphinx.ext.viewcode", "sphinx.ext.mathjax", "sphinx.ext.napoleon"

[tk0miya]

@keewis Could you check this please? I think this is related to convert_numpy_type_spec.

[keewis]

napoleon converts the docstring to

Establish a shared lock to the resource.

:Parameters: * **timeout** (:class:`Union[float`, :class:`Literal[```"default"``:class:`]]`, *optional*) -- Absolute time period (in milliseconds) that a resource waits to get
               unlocked by the locking session before returning an error.
               Defaults to "default" which means use self.timeout.
             * **requested_key** (:class:`Optional[str]`, *optional*) -- Access key used by another session with which you want your session
               to share a lock or None to generate a new shared access key.

:returns: *str* -- A new shared access key if requested_key is None, otherwise, same
          value as the requested_key

which I guess happens because I never considered typehints when I wrote the preprocessor. To be clear, type hints are not part of the format guide, but then again it also doesn't say they can't be used.

If we allow type hints, we probably want to link those types and thus should extend the preprocessor. Since that would be a new feature, I guess we shouldn't include that in a bugfix release.

For now, I suggest we fix this by introducing a setting that allows opting out of the type preprocessor (could also be opt-in).

[StrikerRUS]

Faced the same issue in our builds yesterday.

Warning, treated as error:
/home/travis/build/microsoft/LightGBM/docs/../python-package/lightgbm/basic.py:docstring of lightgbm.Booster.dump_model:12:Inline literal start-string without end-string.

conf.py: https://github.com/microsoft/LightGBM/blob/master/docs/conf.py
Logs: https://travis-ci.org/github/microsoft/LightGBM/jobs/716228303

One of the "problem" docstrings: https://github.com/microsoft/LightGBM/blob/ee8ec182010c570c6371a5fc68ab9f4da9c6dc74/python-package/lightgbm/basic.py#L2762-L2782

[keewis]

that's a separate issue: you're using a unsupported notation for default. Supported are currently default <obj> and default: <obj>, while you are using optional (default=<obj>). To be fair, this is currently not standardized, see numpy/numpydoc#289.

Edit: in particular, the type preprocessor chokes on something like string, optional (default="split"), which becomes:

:class:`string`, :class:`optional (default=```"split"``:class:`)`

so it splits the default notation into optional (default=, "split", and )

However, the temporary fix is the same: deactivate the type preprocessor using a new setting. For a long term fix we'd first need to update the numpydoc format guide.

@tk0miya, should I send in a PR that adds that setting?

[tk0miya]

@keewis Yes, please.

If we allow type hints, we probably want to link those types and thus should extend the preprocessor. Since that would be a new feature, I guess we shouldn't include that in a bugfix release.

I think the new option is needed to keep compatibility for some users. So it must be released as a bugfix release. So could you send a PR to 3.2.x branch? I'm still debating which is better to enable or disable the numpy type feature by default. But it should be controlled via user settings.

[MatthieuDartiailh]

Even if type hints are not clearly supported, the numpy docs format (https://numpydoc.readthedocs.io/en/latest/format.html) mention this as being valid entry for a parameter order : {'C', 'F', 'A'}. Correct me if I am missing something but the error arises from the presence of " and is not related to [] in type hints. As a consequence I assume this example would break in the same way (got no time to test though).

[keewis]

no, I definitely made sure value sets and strings are supported: https://github.com/sphinx-doc/sphinx/blob/38b868cc0d0583d9a58496cd121f0bc345bf9eaa/tests/test_ext_napoleon_docstring.py#L2145-L2176
Though of course I can't be sure there is no edge case that breaks the conversion code (I didn't use a fuzzer to check that).

@tk0miya, I just realized we could probably treat [ and ] as normal delimiters (like or) and just make sure the number of opening and closing brackets match (maybe also that the token after ] must be another delimiter).

[MatthieuDartiailh]

Thanks for clarifying the issue I assumed that [ was not causing an issue since I had to remove " to fix the issue. I think that treating [ as a normal delimiter would go in the right direction. It would provide the benefits of the new pre-processor while avoiding the regression for type hints.

Silly question but moving forward would it make sense to be able to "complete" the docstring when building the documentation from the type annotations to avoid duplicating everything ? Is this planned ?

[tk0miya]

I merged #8095 now. So this warning will be fixed on next release.

[tk0miya]

@MatthieuDartiailh

Silly question but moving forward would it make sense to be able to "complete" the docstring when building the documentation from the type annotations to avoid duplicating everything ? Is this planned ?

What do you mean "complete"?

IMO, type hints (type annotations) is a good friend of developers. They help to our development. Additionally, autodoc detects them and adds them to the document automatically. So I prefer to write them as type annotations instead of docstrings.

[MatthieuDartiailh]

My idea was to stop adding the types in the docstrings and get them directly from the hints to avoid duplication. Somebody pointed me to that project which seems like it can do it: https://github.com/agronholm/sphinx-autodoc-typehints. However the same person pointed out the fact that some tools (IPython) display the source docstring which would then be less readable. I guess what I am looking for is a good linter/formatter to avoid discrepancies between type annotations and docstrings.

Anyway thanks for your help.