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 with return annotation when using @contextmanager #68

Closed
haakonvt opened this issue Aug 18, 2023 · 9 comments
Closed

Bug with return annotation when using @contextmanager #68

haakonvt opened this issue Aug 18, 2023 · 9 comments

Comments

@haakonvt
Copy link

haakonvt commented Aug 18, 2023

I use contextmanager in several places in my testing utilities, and one of these functions are part of the public API, meaning it's documented. I struggle to write it in a manner that does not raise any errors from pydoclint:

from contextlib import contextmanager
from typing import Iterator


@contextmanager
def stuff_maker() -> Iterator[int]:
    """Text

    Yields:
        Iterator[int]: The stuff
    """
    yield 5

Running this file gives me a DOC203 error:

$ pydoclint --style=google aaaa.py
    6: DOC203: Function `stuff_maker` return type(s) in docstring not consistent with the return annotation. Return annotation has 1 type(s); docstring return section has 0 type(s).

I have tried several variations, but all lead to at least one error 🤔 Any suggestions?

@haakonvt
Copy link
Author

Changing Iterator to Generator solved it 👍

@haakonvt
Copy link
Author

...using iterator should work though 😉

@jsh9
Copy link
Owner

jsh9 commented Aug 18, 2023

...using iterator should work though 😉

Hi @haakonvt ,

You touched on a point that many other people may agree with (or may be confused about).

I sort of have the opinion that when we use yield, the return type should be a Generator rather than an Iterator. In this comment from another issue (#15 (comment)), I showed that a Generator is always an Iterator, but not the other way around.

That's why when pydoclint sees yield, it expects Generator rather than Iterator in the return type annotation.

You may be wondering: when would an Iterator be appropriate as a return annotation? Here is an example (taken from this comment: #15 (comment)):

from typing import Any, Iterator, Tuple, List

def zip_lists(list1: List[Any], list2: List[Any]) -> Iterator[Tuple[Any, Any]]:
    return zip(list1, list2)

@haakonvt
Copy link
Author

Thanks for the detailed answer. I think your logic is sound (and I don't mind annotating as Generator), but you can also turn it around: All functions returning an Iterator with a yield statement present are generators (afaik).

@jsh9
Copy link
Owner

jsh9 commented Aug 18, 2023

but you can also turn it around: All functions returning an Iterator with a yield statement present are generators (afaik).

Yes, this is correct. But this would introduce an ambiguity: when pydoclint sees -> Iterator[...] in the function signature, it doesn't know what to check in the docstring: should there be a "Returns" section, or should there be a "Yields" section?

You might wonder: can we check the existence of return statements or yield statements in the function body to decide? Unfortunately, there is nothing that stops people from using return statements and yield statements in the same function (although it is probably not the best practice).

Therefore, the only unambiguous way becomes the following:

  • If the return annotation in the signature is Generator[..., ..., ...], there should be a "Yields" section in the docstring
  • If the return annotation in the signature is Iterator[...], there should be a "Returns" section in the docstring

There is some adoption cost to this: users (such as you) would need to update a lot of the return annotations from Iterator to Generator. But this is only a one-time cost, and it's actually more explicit (one of Python's zen is: explicit is better than implicit).

@jsh9
Copy link
Owner

jsh9 commented Aug 18, 2023

That said, the fact that you saw this violation ("DOC203: Function stuff_maker return type(s) in docstring not consistent with the return annotation. Return annotation has 1 type(s); docstring return section has 0 type(s).") means that my violation explanations are not self explanatory.

I'll work on a code change to improve the explanation.

@jsh9
Copy link
Owner

jsh9 commented Aug 19, 2023

I made a code change (#70) to explicitly show this self explanatory violation:

DOC405: Method ... has "yield" statements, but the return signature
is Iterator. Please use Generator instead. (Read more about this topic
here: https://jsh9.github.io/pydoclint/notes_generator_vs_iterator.html )

@haakonvt
Copy link
Author

That is a greatly improved error message 👌

@jsh9
Copy link
Owner

jsh9 commented Aug 19, 2023

This change is included in the new published version, 0.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants