astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.31k stars 1.08k forks source link

DOC403 behaviour with yielding none #13001

Closed jonyscathe closed 1 month ago

jonyscathe commented 2 months ago

[tool.ruff.format] line-ending = 'lf' quote-style = 'single'

[tool.ruff.lint] extend-select = ['D213'] ignore = ['ANN401', 'CPY001', 'D107', 'D203', 'D212', 'E203', 'ISC003', 'PLC1901', 'PLR6104'] select = ['ALL']

[tool.ruff.lint.flake8-implicit-str-concat] allow-multiline = false

[tool.ruff.lint.flake8-quotes] inline-quotes = 'single'

[tool.ruff.lint.flake8-type-checking] strict = true

[tool.ruff.lint.pydocstyle] convention = 'numpy'

[tool.ruff.lint.pylint] max-args = 14 max-statements = 75

* The current Ruff version (`ruff --version`).
 ruff 0.6.1

The above code snippet results in a DOC403 Docstring has a "Yields" section but the function doesn't yield anything.

If the yields section is removed from the docstring then [numpydoc-validation](https://numpydoc.readthedocs.io/en/latest/validation.html) reports:

+---------------------+-------------------+---------+-------------------------+ | file | item | check | description | +=====================+===================+=========+=========================+ | src/conftest.py:135 | conftest.db_setup | YD01 | No Yields section found | +---------------------+-------------------+---------+-------------------------+



I understand that it is debatable about what the docstring should be a for a yield statement that yields None, but I think ruff should probably match the numpydoc-validator tool in this case.
dhruvmanila commented 2 months ago

This seems like a bug. An empty yield isn't considered to be present in the body:

https://github.com/astral-sh/ruff/blob/4d0d3b00cb99e0abb934877dddec1ffcbc488818/crates/ruff_linter/src/rules/pydoclint/rules/check_docstring.rs#L638-L644

@augustelalande just checking in to see if there's a specific reason behind this otherwise we can remove the Some(_) condition from above.

augustelalande commented 2 months ago

I did this on purpose because IMO the yields section should document the yielded value not, the function behaviour associated with the yield. Similar to return None, not needing documentation.

The yield "behaviour" could be documented in the docstring summary.

But if people feel strongly the other way, feel free to change the behaviour.

jonyscathe commented 2 months ago

@augustelalande to be fair I do agree with your logic. I only raised this due to conflicting behaviour with numpydoc's validation tool. Maybe I will raise an issue with numpydoc and see what they say about it.

dhruvmanila commented 2 months ago

I did this on purpose because IMO the yields section should document the yielded value not, the function behaviour associated with the yield. Similar to return None, not needing documentation.

Yeah, that makes sense.

Maybe I will raise an issue with numpydoc and see what they say about it.

@jonyscathe That might be useful, thanks

jonyscathe commented 2 months ago

The numpydoc people do believe that the yield should be documented even if it is none, as it is not really the same thing as returning none (the yield is still doing something). https://github.com/numpy/numpydoc/issues/580

They also have the example that something like:

def yielder():
    for _ in range(10):
        yield

is inherently different to:

def yielder():
    yield

And should be documented as such in the Yields section (10 implicit None's via a generator vs 1).

larsoner commented 2 months ago

... and just to bring the discussion over here to save some a click, I suggested:

It seems reasonable that [these two examples] would both need some Yields section of some sort to tell people about the nature of the returned generator.

It seems natural to put this in a Yields section. But I am not 100% convinced either way. cc @stefanv who was also discussing over in numpydoc.

dhruvmanila commented 2 months ago

@jonyscathe @larsoner Thanks for confirming! I think it makes sense to update the behavior. All we need is to remove the Some(_) match as mentioned in https://github.com/astral-sh/ruff/issues/13001#issuecomment-2297862398.

It seems that pydoclint doesn't raise an issue for the source code mentioned in the PR description.

tjkuson commented 2 months ago

Is the suggestion to report docstring-missing-yields (DOC402) on

@pytest.fixture
def db_setup() -> Generator[None, None, None]:
    """
    Add test user.

    """
    database = Database('my_database_url')
    with database.create_session() as session:
        session.execute(insert(database.Users, [{'id': 1, 'name': 'Bob'}

    yield

    with database.create_session() as session:
        session.execute(delete(database.Users).where(database.Users.id == 1234)

as well?

If it is unclear whether implicit yield None requires a yields section in the docstring, it makes sense to not report a DOC403 upon its absence. However, it might also make sense to not report DOC402 if an implicit yield None is present without an accompanying docstring section — i.e., to permit the above code example. In many situations, an implicit yield None is trivial and only occurs once, and thus a yields section would only exist to document that the object is a generator (already achieved via a typing.Generator annotation).

If we want both DOC403 and DOC402 make exceptions for implicit yields (that is to say, count them in one and ignore in them in the other), then it is more than a one-line code change.

Possibly related to #13062