From 4d36cccd742e27531d78a69dc7b3977c9bfdb329 Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 8 Jun 2020 13:43:09 +0200 Subject: [PATCH 01/13] escape combined args and kwargs for numpy docstrings --- sphinx/ext/napoleon/docstring.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 32edd7f8f..228b8859e 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -876,6 +876,16 @@ class NumpyDocstring(GoogleDocstring): self._directive_sections = ['.. index::'] super().__init__(docstring, config, app, what, name, obj, options) + def _escape_args_and_kwargs(self, name): + if ", " in name: + parts = name.split(", ") + return ", ".join( + super()._escape_args_and_kwargs(part) + for part in parts + ) + + return super()._escape_args_and_kwargs(name) + def _consume_field(self, parse_type: bool = True, prefer_type: bool = False ) -> Tuple[str, str, List[str]]: line = next(self._line_iter) From 15b443c074e2c584e8b3779d174e091cfcaeb729 Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 8 Jun 2020 13:52:46 +0200 Subject: [PATCH 02/13] add a test for combined args and kwargs --- tests/test_ext_napoleon_docstring.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_ext_napoleon_docstring.py b/tests/test_ext_napoleon_docstring.py index 738fd6532..879a2aaa4 100644 --- a/tests/test_ext_napoleon_docstring.py +++ b/tests/test_ext_napoleon_docstring.py @@ -1189,6 +1189,23 @@ class NumpyDocstringTest(BaseDocstringTest): """ Single line summary + Parameters + ---------- + arg1:str + Extended description of arg1 + *args, **kwargs: + Variable length argument list and arbitrary keyword arguments. + """, + """ + Single line summary + + :Parameters: * **arg1** (*str*) -- Extended description of arg1 + * **\\*args, \\*\\*kwargs** -- Variable length argument list and arbitrary keyword arguments. + """ + ), ( + """ + Single line summary + Yield ----- str From d5f552685c42264bf9bdb5a32d692fa7688c1f81 Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 8 Jun 2020 13:53:11 +0200 Subject: [PATCH 03/13] use a alias of the implementation of the base class and add type hints --- sphinx/ext/napoleon/docstring.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 228b8859e..e6f2c2998 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -876,15 +876,14 @@ class NumpyDocstring(GoogleDocstring): self._directive_sections = ['.. index::'] super().__init__(docstring, config, app, what, name, obj, options) - def _escape_args_and_kwargs(self, name): + def _escape_args_and_kwargs(self, name: str) -> str: + func = super()._escape_args_and_kwargs + if ", " in name: parts = name.split(", ") - return ", ".join( - super()._escape_args_and_kwargs(part) - for part in parts - ) + return ", ".join(func(part) for part in parts) - return super()._escape_args_and_kwargs(name) + return func(name) def _consume_field(self, parse_type: bool = True, prefer_type: bool = False ) -> Tuple[str, str, List[str]]: From 95ec45b562392c63c7d1eddd945eaf177b4f747d Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 8 Jun 2020 14:19:03 +0200 Subject: [PATCH 04/13] check that we can only combine *args with **kwargs and vice versa --- sphinx/ext/napoleon/docstring.py | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index e6f2c2998..2fa40ddb2 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -18,13 +18,16 @@ from typing import Any, Callable, Dict, List, Tuple, Union from sphinx.application import Sphinx from sphinx.config import Config as SphinxConfig from sphinx.ext.napoleon.iterators import modify_iter -from sphinx.locale import _ +from sphinx.locale import _, __ +from sphinx.util import logging if False: # For type annotation from typing import Type # for python3.5.1 +logger = logging.getLogger(__name__) + _directive_regex = re.compile(r'\.\. \S+::') _google_section_regex = re.compile(r'^(\s|\w)+:\s*$') _google_typed_arg_regex = re.compile(r'\s*(.+?)\s*\(\s*(.*[^\s]+)\s*\)') @@ -880,8 +883,30 @@ class NumpyDocstring(GoogleDocstring): func = super()._escape_args_and_kwargs if ", " in name: - parts = name.split(", ") - return ", ".join(func(part) for part in parts) + args, kwargs, *rest = name.split(", ") + + is_args = args[:1] == "*" and len([c for c in args if c == "*"]) == 1 + is_kwargs = kwargs[:2] == "**" and len([c for c in kwargs if c == "*"]) == 2 + + if is_args or is_kwargs and not (is_args and is_kwargs): + name_ = args if is_args else kwargs + other = "*args" if not is_args else "**kwargs" + logger.warning( + __("can only combine parameters of form %s with %s: %s"), + name_, + other, + name, + location=None, + ) + elif is_args and is_kwargs and rest: + logger.warning( + __("cannot combine %s and %s with more parameters: %s"), + args, + kwargs, + name, + location=None, + ) + return ", ".join([func(args), func(kwargs)]) return func(name) From 0adc7999f747d75dd37c9b0ab1b1f2c5e69d738b Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 12 Jul 2020 01:29:38 +0200 Subject: [PATCH 05/13] add location information and make sure args and kwargs cannot be swapped --- sphinx/ext/napoleon/docstring.py | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 2fa40ddb2..1eb0a35cb 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -885,10 +885,24 @@ class NumpyDocstring(GoogleDocstring): if ", " in name: args, kwargs, *rest = name.split(", ") - is_args = args[:1] == "*" and len([c for c in args if c == "*"]) == 1 - is_kwargs = kwargs[:2] == "**" and len([c for c in kwargs if c == "*"]) == 2 + def check_args(s): + return s[:1] == "*" and len([c for c in s if c == "*"]) == 1 - if is_args or is_kwargs and not (is_args and is_kwargs): + def check_kwargs(s): + return s[:2] == "**" and len([c for c in s if c == "*"]) == 2 + + is_args = check_args(args) + is_kwargs = check_kwargs(kwargs) + + location_name = "docstring of {}".format(self._name) + location = ":".join([self._obj.__code__.co_filename, location_name, ""]) + if check_args(kwargs) or check_kwargs(args): + logger.warning( + __("wrong order of *args and **kwargs: %s"), + name, + location=location, + ) + elif is_args or is_kwargs and not (is_args and is_kwargs): name_ = args if is_args else kwargs other = "*args" if not is_args else "**kwargs" logger.warning( @@ -896,7 +910,7 @@ class NumpyDocstring(GoogleDocstring): name_, other, name, - location=None, + location=location, ) elif is_args and is_kwargs and rest: logger.warning( @@ -904,7 +918,7 @@ class NumpyDocstring(GoogleDocstring): args, kwargs, name, - location=None, + location=location, ) return ", ".join([func(args), func(kwargs)]) From e37bfaea83bf743355ffa16413695930226cfe92 Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 12 Jul 2020 01:37:20 +0200 Subject: [PATCH 06/13] add a method to get the current docstring's location --- sphinx/ext/napoleon/docstring.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 1eb0a35cb..ac8d05bc9 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -879,6 +879,12 @@ class NumpyDocstring(GoogleDocstring): self._directive_sections = ['.. index::'] super().__init__(docstring, config, app, what, name, obj, options) + def _get_location(self): + filepath = self._obj.__code__.co_filename + name = self._name + + return "{}: docstring of {}:".format(filepath, name) + def _escape_args_and_kwargs(self, name: str) -> str: func = super()._escape_args_and_kwargs @@ -894,8 +900,7 @@ class NumpyDocstring(GoogleDocstring): is_args = check_args(args) is_kwargs = check_kwargs(kwargs) - location_name = "docstring of {}".format(self._name) - location = ":".join([self._obj.__code__.co_filename, location_name, ""]) + location = self._get_location() if check_args(kwargs) or check_kwargs(args): logger.warning( __("wrong order of *args and **kwargs: %s"), From 0bb05031e43a8f0861be3db78e417ae4ef2ddefd Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 12 Jul 2020 02:22:03 +0200 Subject: [PATCH 07/13] make sure to return None if both path and name are None i.e. most likely a test --- sphinx/ext/napoleon/docstring.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index ac8d05bc9..f34912c38 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -880,9 +880,12 @@ class NumpyDocstring(GoogleDocstring): super().__init__(docstring, config, app, what, name, obj, options) def _get_location(self): - filepath = self._obj.__code__.co_filename + filepath = self._obj.__code__.co_filename if self._obj is not None else None name = self._name + if filepath is None and name is None: + return None + return "{}: docstring of {}:".format(filepath, name) def _escape_args_and_kwargs(self, name: str) -> str: From 5f12d5b476d2f14fc6ff9f609005e36268dc8bea Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 12 Jul 2020 16:18:04 +0200 Subject: [PATCH 08/13] use inspect.getfile instead --- sphinx/ext/napoleon/docstring.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index f34912c38..58d86365b 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -879,8 +879,8 @@ class NumpyDocstring(GoogleDocstring): self._directive_sections = ['.. index::'] super().__init__(docstring, config, app, what, name, obj, options) - def _get_location(self): - filepath = self._obj.__code__.co_filename if self._obj is not None else None + def _get_location(self) -> str: + filepath = inspect.getfile(self._obj) if self._obj is not None else None name = self._name if filepath is None and name is None: From b66f3e143e6b10fda4d140edf13d7cfaf1144d50 Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 13 Jul 2020 15:40:29 +0200 Subject: [PATCH 09/13] fix the conditions for warning --- sphinx/ext/napoleon/docstring.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 58d86365b..fc7a3ea95 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -904,13 +904,13 @@ class NumpyDocstring(GoogleDocstring): is_kwargs = check_kwargs(kwargs) location = self._get_location() - if check_args(kwargs) or check_kwargs(args): + if (not is_args and check_args(kwargs)) and (not is_kwargs and check_kwargs(args)): logger.warning( __("wrong order of *args and **kwargs: %s"), name, location=location, ) - elif is_args or is_kwargs and not (is_args and is_kwargs): + elif (is_args or is_kwargs) and not (is_args and is_kwargs): name_ = args if is_args else kwargs other = "*args" if not is_args else "**kwargs" logger.warning( From 467533ffe87ec0aba9a17c8a567df555d9a448f7 Mon Sep 17 00:00:00 2001 From: Keewis Date: Mon, 13 Jul 2020 15:49:03 +0200 Subject: [PATCH 10/13] add tests to make sure the warning is raised --- sphinx/ext/napoleon/docstring.py | 2 +- tests/test_ext_napoleon_docstring.py | 45 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index fc7a3ea95..691139a48 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -883,7 +883,7 @@ class NumpyDocstring(GoogleDocstring): filepath = inspect.getfile(self._obj) if self._obj is not None else None name = self._name - if filepath is None and name is None: + if filepath is None and not name: return None return "{}: docstring of {}:".format(filepath, name) diff --git a/tests/test_ext_napoleon_docstring.py b/tests/test_ext_napoleon_docstring.py index 879a2aaa4..e24c7ed78 100644 --- a/tests/test_ext_napoleon_docstring.py +++ b/tests/test_ext_napoleon_docstring.py @@ -9,11 +9,15 @@ :license: BSD, see LICENSE for details. """ +import re from collections import namedtuple +from contextlib import contextmanager from inspect import cleandoc from textwrap import dedent from unittest import TestCase, mock +import pytest + from sphinx.ext.napoleon import Config from sphinx.ext.napoleon.docstring import GoogleDocstring, NumpyDocstring @@ -2003,3 +2007,44 @@ Do as you please :kwtype gotham_is_yours: None """ self.assertEqual(expected, actual) + + +@contextmanager +def warns(warning, match): + match_re = re.compile(match) + try: + yield warning + finally: + raw_warnings = warning.getvalue() + warnings = [w for w in raw_warnings.split("\n") if w.strip()] + + assert len(warnings) == 1 and all(match_re.match(w) for w in warnings) + + +class TestNumpyDocstring: + @pytest.mark.parametrize( + ["spec", "pattern"], + ( + pytest.param("*args, *kwargs", ".+: can only combine parameters of form", id="two args"), + pytest.param("**args, **kwargs", ".+: can only combine parameters of form", id="two kwargs"), + pytest.param( + "*args, **kwargs, other_parameter", + ".+: cannot combine .+ and .+ with more parameters", + id="more parameters", + ), + pytest.param("**kwargs, *args", r".+: wrong order of .+ and .+", id="swapped parameters"), + ) + ) + def test_invalid_combined_args_and_kwargs(self, spec, pattern, app, warning): + docstring = dedent( + """\ + Parameters + ---------- + {} + variable args list and arbitrary keyword arguments + """ + ).format(spec) + config = Config() + + with warns(warning, match=pattern): + str(NumpyDocstring(docstring, config, app, "method")) From 2a3f6e4d7009cc6c6105e45ae07c135e9455ecb9 Mon Sep 17 00:00:00 2001 From: Keewis Date: Sun, 2 Aug 2020 19:28:01 +0200 Subject: [PATCH 11/13] don't try to split along ', ' --- sphinx/util/docfields.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sphinx/util/docfields.py b/sphinx/util/docfields.py index c07bc7f66..55c00c2ec 100644 --- a/sphinx/util/docfields.py +++ b/sphinx/util/docfields.py @@ -322,6 +322,8 @@ class DocFieldTransformer: if typedesc.is_typed: try: argtype, argname = fieldarg.split(None, 1) + if argtype.endswith(","): + raise ValueError except ValueError: pass else: From e79cd79cd2d1e46d57fd6b941bd6348eb53bdb17 Mon Sep 17 00:00:00 2001 From: Keewis Date: Wed, 5 Aug 2020 14:28:32 +0200 Subject: [PATCH 12/13] revert the change to DocFieldTransformer --- sphinx/util/docfields.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/sphinx/util/docfields.py b/sphinx/util/docfields.py index 55c00c2ec..c07bc7f66 100644 --- a/sphinx/util/docfields.py +++ b/sphinx/util/docfields.py @@ -322,8 +322,6 @@ class DocFieldTransformer: if typedesc.is_typed: try: argtype, argname = fieldarg.split(None, 1) - if argtype.endswith(","): - raise ValueError except ValueError: pass else: From 849d3c18a7498fd72faa29064a0c813ed457a6af Mon Sep 17 00:00:00 2001 From: Keewis Date: Wed, 5 Aug 2020 19:22:43 +0200 Subject: [PATCH 13/13] remove the syntax checks from the escape method and update the tests --- sphinx/ext/napoleon/docstring.py | 42 ++-------------------------- tests/test_ext_napoleon_docstring.py | 31 ++++++-------------- 2 files changed, 12 insertions(+), 61 deletions(-) diff --git a/sphinx/ext/napoleon/docstring.py b/sphinx/ext/napoleon/docstring.py index 2e0acd867..df1782934 100644 --- a/sphinx/ext/napoleon/docstring.py +++ b/sphinx/ext/napoleon/docstring.py @@ -1090,45 +1090,9 @@ class NumpyDocstring(GoogleDocstring): func = super()._escape_args_and_kwargs if ", " in name: - args, kwargs, *rest = name.split(", ") - - def check_args(s): - return s[:1] == "*" and len([c for c in s if c == "*"]) == 1 - - def check_kwargs(s): - return s[:2] == "**" and len([c for c in s if c == "*"]) == 2 - - is_args = check_args(args) - is_kwargs = check_kwargs(kwargs) - - location = self._get_location() - if (not is_args and check_args(kwargs)) and (not is_kwargs and check_kwargs(args)): - logger.warning( - __("wrong order of *args and **kwargs: %s"), - name, - location=location, - ) - elif (is_args or is_kwargs) and not (is_args and is_kwargs): - name_ = args if is_args else kwargs - other = "*args" if not is_args else "**kwargs" - logger.warning( - __("can only combine parameters of form %s with %s: %s"), - name_, - other, - name, - location=location, - ) - elif is_args and is_kwargs and rest: - logger.warning( - __("cannot combine %s and %s with more parameters: %s"), - args, - kwargs, - name, - location=location, - ) - return ", ".join([func(args), func(kwargs)]) - - return func(name) + return ", ".join(func(param) for param in name.split(", ")) + else: + return func(name) def _consume_field(self, parse_type: bool = True, prefer_type: bool = False ) -> Tuple[str, str, List[str]]: diff --git a/tests/test_ext_napoleon_docstring.py b/tests/test_ext_napoleon_docstring.py index 6421e5fa2..23935925b 100644 --- a/tests/test_ext_napoleon_docstring.py +++ b/tests/test_ext_napoleon_docstring.py @@ -2238,28 +2238,15 @@ class TestNumpyDocstring: _token_type(token) @pytest.mark.parametrize( - ["spec", "pattern"], + ("name", "expected"), ( - pytest.param("*args, *kwargs", ".+: can only combine parameters of form", id="two args"), - pytest.param("**args, **kwargs", ".+: can only combine parameters of form", id="two kwargs"), - pytest.param( - "*args, **kwargs, other_parameter", - ".+: cannot combine .+ and .+ with more parameters", - id="more parameters", - ), - pytest.param("**kwargs, *args", r".+: wrong order of .+ and .+", id="swapped parameters"), - ) + ("x, y, z", "x, y, z"), + ("*args, **kwargs", r"\*args, \*\*kwargs"), + ("*x, **y", r"\*x, \*\*y"), + ), ) - def test_invalid_combined_args_and_kwargs(self, spec, pattern, app, warning): - docstring = dedent( - """\ - Parameters - ---------- - {} - variable args list and arbitrary keyword arguments - """ - ).format(spec) - config = Config() + def test_escape_args_and_kwargs(self, name, expected): + numpy_docstring = NumpyDocstring("") + actual = numpy_docstring._escape_args_and_kwargs(name) - with warns(warning, match=pattern): - str(NumpyDocstring(docstring, config, app, "method")) + assert actual == expected