Skip to content
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

bug: Google docstrings: no support for non-multiple or non-named values in Yields section #263

Closed
the-13th-letter opened this issue May 2, 2024 · 3 comments · Fixed by #322
Assignees
Labels
griffe: google Related to Google-style docstrings

Comments

@the-13th-letter
Copy link
Contributor

Description of the bug

In Google-style docstrings, Griffe (and mkdocstrings-python) don't treat Yields and Returns sections identically when it comes to returning multiple and/or named items. In particular, for Returns sections, the returns_multiple_items and returns_named_values configuration options exist and do as advertised. For Yields sections, no such dedicated options exist, and those options are ignored. As a consequence, for multiple values, there is no way to document the yielded tuple of values as a tuple; it must be faked via a single item tuple, the type repeated (actually: overruled) in the docstring, and the description must be written with hanging indentation.

Note: The Google style guide, at least as of version 8487c08, treats "Yields:" and "Returns:" identically and requires them to document a tuple of returned values as a tuple, explicitly forbidding the numpy-style named tuple return values.

To Reproduce

Given the following example module yielding_test

"""Test "Yields:" sections in Google-style docstrings."""

from __future__ import annotations

from collections.abc import Iterator

def return_one() -> str:
    """XXX

    Returns:
        Returns one item.  Returns one item.  Returns one item.  Returns one
        item.  Returns one item.  Returns one item.

    """
    return "one"

def return_two() -> tuple[str, int]:
    """XXX

    Returns:
        retval: Returns two items.  Returns two items.  Returns two items.
        Returns two items.  Returns two items.  Returns two items.

    """
    return "one", 2

def yield_one() -> Iterator[str]:
    """XXX

    Yields:
        Yields one item.  Yields one item.  Yields one item.  Yields one
        item.  Yields one item.  Yields one item.

    """
    yield "one"

def yield_two() -> Iterator[tuple[str, int]]:
    """XXX

    Yields:
        yieldval: Yields two items.  Yields two items.  Yields two
        items.  Yields two items.  Yields two items.  Yields two items.

    """
    yield "one", 2

and parsed with options returns_multiple_items and returns_named_value as false, the dump from Griffe shows that yield_one supposedly has two return values (reusing the type annotation for the "second" return value) and that yield_two also has two return values, the second one being unnamed.

Full traceback

Full Griffe dump
$ PYTHONPATH=. griffe dump -d google -D '{"returns_multiple_items": false, "returns_named_value": false}' -f -LDEBUG yielding_test
INFO       Loading package yielding_test
DEBUG      Found yielding_test: loading
DEBUG      Loading path /tmp/XYZ/yielding_test.py
INFO       Finished loading packages
{
  "yielding_test": {
    "kind": "module",
    "name": "yielding_test",
    "path": "yielding_test",
    "filepath": "/tmp/XYZ/yielding_test.py",
    "relative_filepath": "yielding_test.py",
    "relative_package_filepath": "XYZ/yielding_test.py",
    "docstring": {
      "value": "Test \"Yields:\" sections in Google-style docstrings.",
      "lineno": 1,
      "endlineno": 1,
      "parsed": [
        {
          "kind": "text",
          "value": "Test \"Yields:\" sections in Google-style docstrings."
        }
      ]
    },
    "labels": [],
    "members": [
      {
        "kind": "alias",
        "name": "annotations",
        "target_path": "__future__.annotations",
        "path": "yielding_test.annotations",
        "lineno": 3,
        "endlineno": 3
      },
      {
        "kind": "alias",
        "name": "Iterator",
        "target_path": "collections.abc.Iterator",
        "path": "yielding_test.Iterator",
        "lineno": 5,
        "endlineno": 5
      },
      {
        "kind": "function",
        "name": "return_one",
        "path": "yielding_test.return_one",
        "filepath": "/tmp/XYZ/yielding_test.py",
        "relative_filepath": "yielding_test.py",
        "relative_package_filepath": "XYZ/yielding_test.py",
        "lineno": 7,
        "endlineno": 15,
        "docstring": {
          "value": "XXX\n\nReturns:\n    Returns one item.  Returns one item.  Returns one item.  Returns one\n    item.  Returns one item.  Returns one item.",
          "lineno": 8,
          "endlineno": 14,
          "parsed": [
            {
              "kind": "text",
              "value": "XXX"
            },
            {
              "kind": "returns",
              "value": [
                {
                  "name": "",
                  "annotation": {
                    "name": "str",
                    "cls": "ExprName"
                  },
                  "description": "Returns one item.  Returns one item.  Returns one item.  Returns one\nitem.  Returns one item.  Returns one item."
                }
              ]
            }
          ]
        },
        "labels": [],
        "members": [],
        "decorators": [],
        "parameters": [],
        "returns": {
          "name": "str",
          "cls": "ExprName"
        }
      },
      {
        "kind": "function",
        "name": "return_two",
        "path": "yielding_test.return_two",
        "filepath": "/tmp/XYZ/yielding_test.py",
        "relative_filepath": "yielding_test.py",
        "relative_package_filepath": "XYZ/yielding_test.py",
        "lineno": 17,
        "endlineno": 25,
        "docstring": {
          "value": "XXX\n\nReturns:\n    retval: Returns two items.  Returns two items.  Returns two items.\n    Returns two items.  Returns two items.  Returns two items.",
          "lineno": 18,
          "endlineno": 24,
          "parsed": [
            {
              "kind": "text",
              "value": "XXX"
            },
            {
              "kind": "returns",
              "value": [
                {
                  "name": "",
                  "annotation": {
                    "name": "retval",
                    "cls": "ExprName"
                  },
                  "description": "Returns two items.  Returns two items.  Returns two items.\nReturns two items.  Returns two items.  Returns two items."
                }
              ]
            }
          ]
        },
        "labels": [],
        "members": [],
        "decorators": [],
        "parameters": [],
        "returns": {
          "left": {
            "name": "tuple",
            "cls": "ExprName"
          },
          "slice": {
            "elements": [
              {
                "name": "str",
                "cls": "ExprName"
              },
              {
                "name": "int",
                "cls": "ExprName"
              }
            ],
            "implicit": true,
            "cls": "ExprTuple"
          },
          "cls": "ExprSubscript"
        }
      },
      {
        "kind": "function",
        "name": "yield_one",
        "path": "yielding_test.yield_one",
        "filepath": "/tmp/XYZ/yielding_test.py",
        "relative_filepath": "yielding_test.py",
        "relative_package_filepath": "XYZ/yielding_test.py",
        "lineno": 27,
        "endlineno": 35,
        "docstring": {
          "value": "XXX\n\nYields:\n    Yields one item.  Yields one item.  Yields one item.  Yields one\n    item.  Yields one item.  Yields one item.",
          "lineno": 28,
          "endlineno": 34,
          "parsed": [
            {
              "kind": "text",
              "value": "XXX"
            },
            {
              "kind": "yields",
              "value": [
                {
                  "name": "",
                  "annotation": {
                    "name": "str",
                    "cls": "ExprName"
                  },
                  "description": "Yields one item.  Yields one item.  Yields one item.  Yields one"
                },
                {
                  "name": "",
                  "annotation": {
                    "name": "str",
                    "cls": "ExprName"
                  },
                  "description": "item.  Yields one item.  Yields one item."
                }
              ]
            }
          ]
        },
        "labels": [],
        "members": [],
        "decorators": [],
        "parameters": [],
        "returns": {
          "left": {
            "name": "Iterator",
            "cls": "ExprName"
          },
          "slice": {
            "name": "str",
            "cls": "ExprName"
          },
          "cls": "ExprSubscript"
        }
      },
      {
        "kind": "function",
        "name": "yield_two",
        "path": "yielding_test.yield_two",
        "filepath": "/tmp/XYZ/yielding_test.py",
        "relative_filepath": "yielding_test.py",
        "relative_package_filepath": "XYZ/yielding_test.py",
        "lineno": 37,
        "endlineno": 45,
        "docstring": {
          "value": "XXX\n\nYields:\n    yieldval: Yields two items.  Yields two items.  Yields two\n    items.  Yields two items.  Yields two items.  Yields two items.",
          "lineno": 38,
          "endlineno": 44,
          "parsed": [
            {
              "kind": "text",
              "value": "XXX"
            },
            {
              "kind": "yields",
              "value": [
                {
                  "name": "yieldval",
                  "annotation": {
                    "name": "str",
                    "cls": "ExprName"
                  },
                  "description": "Yields two items.  Yields two items.  Yields two"
                },
                {
                  "name": "",
                  "annotation": {
                    "name": "int",
                    "cls": "ExprName"
                  },
                  "description": "items.  Yields two items.  Yields two items.  Yields two items."
                }
              ]
            }
          ]
        },
        "labels": [],
        "members": [],
        "decorators": [],
        "parameters": [],
        "returns": {
          "left": {
            "name": "Iterator",
            "cls": "ExprName"
          },
          "slice": {
            "left": {
              "name": "tuple",
              "cls": "ExprName"
            },
            "slice": {
              "elements": [
                {
                  "name": "str",
                  "cls": "ExprName"
                },
                {
                  "name": "int",
                  "cls": "ExprName"
                }
              ],
              "implicit": true,
              "cls": "ExprTuple"
            },
            "cls": "ExprSubscript"
          },
          "cls": "ExprSubscript"
        }
      }
    ]
  }
}

Note that the docstring.parsed[1].value arrays contain one item for the returns_* functions, and two items for the yields_* functions. The latter is unexpected.

Expected behavior

On a high level, I expect the output for the yields_one and yields_two to be similar to the respective return_one and return_two output (except for "kind": "returns" vs. "kind": "yields").

On a lower level, I expect the returns_multiple_items and returns_named_value options to also be respected when parsing Yields sections.

Environment information

Running Griffe v0.44.0 on Linux, installed via PyPI.

Additional context

Support for non-multiple return values and named return values seems to have originated in #137 (0.35.0, 2023-08-24) and #196 (0.36.0, 2023-09-01). In both cases only the Returns section handling was adapted, and Yields section handling was left untouched.

Additional comments

I do not advocate adding additional configuration options yields_multiple_items/yields_named_value or similar to handle this case. It makes sense to reuse the existing returns_* options, because Google style treats Returns and Yields so similarly.

I tried my hand at copying the respective code from src/griffe/docstrings/google.py:_read_returns_section to _read_yields_section, but I had trouble setting up new tests in tests/test_docstrings/test_google.py. I don't know enough about Griffe's internals (or pytest) to properly debug whether the copied code is bad or whether my tests are bad. (Or both.) So, no pull request, just a bug report. Sorry.

@the-13th-letter the-13th-letter added the unconfirmed This bug was not reproduced yet label May 2, 2024
@pawamoy
Copy link
Member

pawamoy commented May 3, 2024

Hey again @the-13th-letter, thanks for the great report. I agree with the suggestion to make yields and returns consistent. Maybe also receives?

@pawamoy pawamoy added griffe: google Related to Google-style docstrings and removed unconfirmed This bug was not reproduced yet labels May 3, 2024
@the-13th-letter
Copy link
Contributor Author

Hey again @the-13th-letter, thanks for the great report. I agree with the suggestion to make yields and returns consistent.

:)

Maybe also receives?

I thought about that as well, but I don't have a good case for this. Just a couple of disjointed random thoughts.

  • Google's style guide doesn't talk about generator input via generator.send(...) at all. So it's anyone's guess how to properly format this or even talk about this in a valid Google-style Python program.

  • The Receives block comes from numpydoc. So it's much more important that it is compatible with NumPy's style than with Google's style. numpydoc currently (v1.8.0rc0) says: “Since, like for Yields and Returns, a single object is always passed to the method, this may describe either the single parameter, or positional arguments passed as a tuple.” So there may be some desire for receives_multiple_items = False.

    • The function analog to Receives is (positional-only) Args/Parameters. I cannot really imagine anyone seriously suggesting that the Args/Parameters block should support not accepting multiple parameters. Why would Receives be any different? So maybe there's no desire receives_multiple_items = False.
  • I find the need for receives_named_value = False somewhat unlikely: you generally do spam, eggs, *parrots = yield ... inside the generator, so you already have “parameter names” (spam, eggs, *parrots) that you probably want to use in the Receives block.

    • But then, perhaps it's a spam = yield ... case where spam is an internal name that you don't want to expose, and now the docs are forcing you to name things anyway...

So yeah, I don't really know what to do with Receives. And I don't think I'm the right person to ask about that either, because I use neither Receives nor the NumPy docstring format in general.

@pawamoy
Copy link
Member

pawamoy commented May 4, 2024

Thanks for your thoughts.

So yeah, I don't really know what to do with Receives. And I don't think I'm the right person to ask about that either, because I use neither Receives nor the NumPy docstring format in general.

Then I'd vote we make "Receives" consistent with the rest, for the sake of consistency. Both Google-style and Numpydoc-style are underspecified, so I always allowed myself some wiggle-room with the goal of making both styles more similar, structurally speaking.

pawamoy pushed a commit that referenced this issue Sep 9, 2024
…s parsing

The Returns, Yields and Receives section parsers only really differ in
their fallbacks and the names of their configuration settings, not so
much in their general parsing behavior and the expected formatting of
the section contents. This commit is an attempt to factor out the
following functionality:

  * Read the section contents as a single block, or as multiple blocks,
    depending on the `multiple` setting.
  * Parse each block's first line as a named parameter, or an unnamed
    parameter, depending on the `named` setting.
  * Unpack `Generator` and `Iterator` types in the return annotation.
    Optionally error out if the return annotation is not of these types.
  * Optionally destructure the return tuple if `multiple` is in effect.

Issue-263: #263
the-13th-letter added a commit to the-13th-letter/derivepassphrase that referenced this issue Sep 29, 2024
[Griffe now handles non-multiple yields and returns similarly.] [YIELDS]
There is thus no need for an empty argument name and type annotation.

[YIELDS]: mkdocstrings/griffe#263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
griffe: google Related to Google-style docstrings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants