Escape combined args kwargs

[keewis]

numpydoc allows combining parameters:

Parameters
---------------
a, b : optional
    factors of the product

but when trying to do that with *args and **kwargs, only the * of args will be escaped. This changes that to also allow the combined form if they are separated by , .

It will also warn if someone tried something like *args, *kwargs or *args, **kwargs, **other_kwargs or **kwargs, *args (which don't really make sense), but the warning lacks location information and I also don't know how to check for warnings in tests.

I wrote this against master but I don't think it is a breaking change. Should I try to rebase onto 3.x?

[tk0miya]

I've still not read this PR yet. But please rebase this into 3.x branch if you feel this is not breaking change. The master branch is used for the next major release (4.0) and it will be released next spring. So it's not good for minor changes.

[tk0miya]

I'll review this later (maybe this weekend)

[keewis]

done. Is the reason why AppVeyor fails due to a change proposed in this PR?

[keewis]

gentle ping, @tk0miya

[tk0miya]

It seems this only works when napoleon_use_param = False. I got following output by default settings (napoleon_use_param = True).

def func():
    """ Single line summary

    Parameters
    ----------
    arg1: str
         Extended description of arg1
    x, y: array_like
         hello
    *args, **kwargs:
        Variable length argument list and arbitrary keyword arguments.
    """

I think this is not a problem around escapes. So we need to improve napoleon module more.

[keewis]

That's true, but I think it's a separate issue: this PR tries to avoid the "Inline strong emphasis start-string without end-string." warnings for *args, **kwargs (which I think is raised otherwise).

[tk0miya]

Yes, it's another issue. But we need to consider how fix it. At present, I think it is better to separate (copy) the attribute entry that contains multiple variables with comma to independent entries. After the change, this fix might not be needed.

[keewis]

that issue is because with napopleon_use_param, combined args get translated to something like:

:param x, y: description
:type x, y: array_like

and it seems that the :param: and :type: field don't work well with the comma.

I still would say that it's fine to work on these separately (this is issue has been there for a long time) and I'd be fine with merging this only after that issue has been fixed. I'd even try to do that myself, but I'd first like to finish #7690 and resolve #7908.

[keewis]

debugging this a bit, the issue is that sphinx.util.docfields.TypedField allows two different styles:

:param foo: description of parameter foo
:type foo:  SomeClass

and

:param SomeClass foo: description of parameter foo

so the DocFieldTransformer will try to always split the name by whitespace, even if it shouldn't:

sphinx/sphinx/util/docfields.py

Lines 323 to 330 in a443538

	try:
	argtype, argname = fieldarg.split(None, 1)
	except ValueError:
	pass
	else:
	types.setdefault(typename, {})[argname] = \
	[nodes.Text(argtype)]
	fieldarg = argname

If instead this was

                try:
                    argtype, argname = fieldarg.split(None, 1)
                    if argtype.endswith(","):
                        raise ValueError
                except ValueError:
                    pass
                else:
                    types.setdefault(typename, {})[argname] = \
                        [nodes.Text(argtype)]
                    fieldarg = argname

using something like x, y would work. Would that be something you'd accept? If so, should I include that in this PR or open a new one?

Edit: I assumed that's okay and that I should include it in this PR (if not please do tell me)

[tk0miya]

I prefer to copy a parameter entry including comma to multiple entries. Because :param: and :type: field expect that its argument is a single name. What do you think about this code?

diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py
index 95fb1e538..243127610 100644
--- a/sphinx/ext/napoleon/docstring.py
+++ b/sphinx/ext/napoleon/docstring.py
@@ -260,14 +260,18 @@ class GoogleDocstring:
         _descs = self.__class__(_descs, self._config).lines()
         return _name, _type, _descs

-    def _consume_fields(self, parse_type: bool = True, prefer_type: bool = False
+    def _consume_fields(self, parse_type: bool = True, prefer_type: bool = False,
-                        ) -> List[Tuple[str, str, List[str]]]:
+                        multiple: bool = False) -> List[Tuple[str, str, List[str]]]:
         self._consume_empty()
         fields = []
         while not self._is_section_break():
             _name, _type, _desc = self._consume_field(parse_type, prefer_type)
             if _name or _type or _desc:
+                if _name:
+                    for name in _name.split(","):
+                        fields.append((name.strip(), _type, _desc,))
+                else:
-                fields.append((_name, _type, _desc,))
+                    fields.append((_name, _type, _desc,))
         return fields

     def _consume_inline_attribute(self) -> Tuple[str, List[str]]:
@@ -675,7 +679,7 @@ class GoogleDocstring:
         return self._format_fields(_('Other Parameters'), self._consume_fields())

     def _parse_parameters_section(self, section: str) -> List[str]:
-        fields = self._consume_fields()
+        fields = self._consume_fields(multiple=True)
         if self._config.napoleon_use_param:
             return self._format_docutils_params(fields)
         else:

[keewis]

As long as modifying GoogleDocstring._consume_fields instead of NumpyDocstring._consume_fields was intendended, the only technical remark I do have is that we probably shouldn't split / copy if napoleon_use_param = False.

Other than that, I don't really like the duplicated entries, because if we mention x and y in the docstring this might become confusing:

    x (array-like) – description of x and y
    y (array-like) – description of x and y

and while I think we're free to deviate, that's not the way numpydoc behaves. So if possible I'd like to keep the entries combined (but I definitely understand the desire to not abuse :param: / :type:).

[keewis]

In order to resolve this: would you merge this without the fix (only the escape of the combined *args, **kwargs)?

We can then investigate / discuss the fix in a separate PR. I think this issue was also reported as #7780?

[tk0miya]

the only technical remark I do have is that we probably shouldn't split / copy if napoleon_use_param = False.

Reasonable. Thank you for your advice. I'll do it in another PR.

[keewis]

I'll do it in another PR.

Just to make sure, I didn't apply the copy / split patch, yet. Is that something you'd do in that separate PR, too?

[tk0miya]

Now I just post a PR for the copy / split patch as #8056

[tk0miya]

Thank you for the quick update. awesome!
I'll review this again tomorrow (with fresh eyes) and merge it soon.

[keewis]

sure. Also, thanks for opening the other PR for me.

[tk0miya]

Merged! Thank you for your continuous contribution!