pydev-guide / pyrepo-copier

Template for creating a new python repository
BSD 3-Clause "New" or "Revised" License
7 stars 3 forks source link

ruff not picking up missing docstrings (e.g. D417) #18

Open jdeschamps opened 1 year ago

jdeschamps commented 1 year ago

In my hands ruff never really helped much when it came to docstrings. For instance, I don't get D417. Here is a python class example in a pyrepo copier example:

class MyClass:
    """A class."""

    def __init__(self, myparam: int) -> None:
        """A constructor."""
        self.myparam = myparam

    def mymethod(self, myarg: int) -> int:
        """A method."""
        if myarg < 0:
            raise ValueError("myarg must be positive.")

        return self.myparam * myarg

With ruff only

.pre-commit-config.yaml:

  - repo: https://github.com/charliermarsh/ruff-pre-commit
    rev: v0.1.3
    hooks:
      - id: ruff
        args: [--fix]

pyproject.toml

[tool.ruff]
line-length = 88
target-version = "py38"
src = ["src"]
# https://beta.ruff.rs/docs/rules/
select = [
    "E",    # style errors
    "W",    # style warnings
    "F",    # flakes
    "D",    # pydocstyle
    "I",    # isort
    "UP",   # pyupgrade
    "C4",   # flake8-comprehensions
    "B",    # flake8-bugbear
    "A001", # flake8-builtins
    "RUF",  # ruff-specific rules
]
# I do this to get numpy-style docstrings AND retain
# D417 (Missing argument descriptions in the docstring)
# otherwise, see:
# https://beta.ruff.rs/docs/faq/#does-ruff-support-numpy-or-google-style-docstrings
# https://github.com/charliermarsh/ruff/issues/2606
ignore = [
    "D100", # Missing docstring in public module
    "D107", # Missing docstring in __init__
    "D203", # 1 blank line required before class docstring
    "D212", # Multi-line docstring summary should start at the first line
    "D213", # Multi-line docstring summary should start at the second line
    "D401", # First line should be in imperative mood
    "D413", # Missing blank line after last section
    "D416", # Section name should end with a colon
]

[tool.ruff.pydocstyle]
convention = "numpy"

[tool.ruff.per-file-ignores]
"tests/*.py" = ["D", "S"]
"setup.py" = ["D"]

This is what I get when running pre-commit:

(pydev) ➜  mytest git:(main) ✗ pre-commit run --all
Validate pyproject.toml..................................................Passed
ruff.....................................................................Passed
black....................................................................Passed
mypy.....................................................................Passed

Minimum ruff

pyproject.toml:

[tool.ruff]
line-length = 88
target-version = "py38"
src = ["src"]
# https://beta.ruff.rs/docs/rules/
select = [
    "D", # pydocstyle
]

[tool.ruff.pydocstyle]
convention = "numpy"

Result:

Validate pyproject.toml..................................................Passed
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

src/my_project/main.py:1:1: D100 Missing docstring in public module
src/my_project/main.py:5:9: D401 First line of docstring should be in imperative mood: "A constructor."
src/my_project/main.py:9:9: D401 First line of docstring should be in imperative mood: "A method."
tests/test_my_project.py:1:1: D100 Missing docstring in public module
tests/test_my_project.py:1:5: D103 Missing docstring in public function
Found 5 errors.

black....................................................................Passed
mypy.....................................................................Passed

With numpydoc (https://github.com/pydev-guide/pyrepo-copier/pull/16)

Adding the following to .pre-commit-config.yaml:

  # check docstrings
  - repo: https://github.com/numpy/numpydoc
    rev: v1.6.0
    hooks:
      - id: numpydoc-validation

And to the pyproject.toml:

# https://numpydoc.readthedocs.io/en/latest/format.html
[tool.numpydoc_validation]
checks = [
    "all",  # report on all checks, except the ones below
    "EX01", # No examples section found
    "SA01", # See Also section not found
    "ES01", # No extended summary found
]
exclude = [ # don't report on objects that match any of these regex
    "test_*",
]
(pydev) ➜  mytest git:(main) ✗ pre-commit run --all
Validate pyproject.toml..................................................Passed
ruff.....................................................................Passed
black....................................................................Passed
mypy.....................................................................Passed
numpydoc-validation......................................................Failed
- hook id: numpydoc-validation
- exit code: 1

+--------------------------+-----------------------+---------+---------------------------------------+
| file                     | item                  | check   | description                           |
+==========================+=======================+=========+=======================================+
| src/my_project/main.py:1 | main                  | GL08    | The object does not have a docstring  |
+--------------------------+-----------------------+---------+---------------------------------------+
| src/my_project/main.py:1 | main.MyClass          | PR01    | Parameters {'myparam'} not documented |
+--------------------------+-----------------------+---------+---------------------------------------+
| src/my_project/main.py:4 | main.MyClass.__init__ | PR01    | Parameters {'myparam'} not documented |
+--------------------------+-----------------------+---------+---------------------------------------+
| src/my_project/main.py:8 | main.MyClass.mymethod | PR01    | Parameters {'myarg'} not documented   |
+--------------------------+-----------------------+---------+---------------------------------------+
| src/my_project/main.py:8 | main.MyClass.mymethod | RT01    | No Returns section found              |
+--------------------------+-----------------------+---------+---------------------------------------+

Note: I used the pyrepo template, but because copier takes the v1.0 tag, I manually updated the pyproject.toml and .pre-commit-config.yaml files (and updated the ruff version).

Any idea what could go wrong with ruff then? If not, I'll open an issue on ruff directly.

tlambert03 commented 1 year ago

pydocstyle D417 doesn't enforce whether or not you add an entire parameters section to your docstring, it lets you know when you've missed one:

def f(x: int, y: str) -> None:
    """Do a thing.

    Parameters
    ----------
    x: int
        first number.
    """
Missing argument description in the docstring for `f`: `y`Ruff[D417](https://docs.astral.sh/ruff/rules/undocumented-param)

For me, that's actually a nice balance. I find it a bit draconian to force me to add a parameters section for everything, particularly when I've already type hinted everything and the parameter names and hints are self explanatory (I feel the same way about the Returns section... should be optional if I've added a type hint, only if I want to add more detail). But, if I do add a Parameters section, then I definitely want to make sure that I keep it updated over time as I add new params, and the current setup ensures that.

tlambert03 commented 1 year ago

admittedly, D417 is the most complicated rule of them all, particularly because it's "off" by default in the numpy pydocstyle convention. (hence why we have all those doc rules in pyproject).

~By the way: the issues you're seeing here is also discussed at https://github.com/astral-sh/ruff/issues/2310~ ... sorry, i think that's a different issue. Might want to ask this specific question there... about enforcing a params section

see also:

jdeschamps commented 1 year ago

I picked D417 because it was mentioned in the pyproject, others is for instance "No Return section" or "No Raise section", which I find quite important (tbf I didn't try to make ruff show those).

But I see your point, not wanting to document self explanatory code! In my case, it was quite helpful because the documentation was half-assed but passing, so it forced me to go over everything...

Thanks for the pointers!

tlambert03 commented 1 year ago

we don't need to close it just yet! :) i mean, this could all be optional after all... I'm not opposed to having an alternative for docstyle, since it's complicated. If we did it, I think we should probably turn it off for ruff though? Reopen this if you want to discuss that

jdeschamps commented 1 year ago

Ahahah sorry, I have a long list of todo these days I am in "crossing asap mode"... I leave the PR close for now though.

I will come back to that later today!

jdeschamps commented 1 year ago

I'm back 😄

So I actually don't have a strong opinion. I imagine that people can indeed get annoyed with the strictness that I enforce with numpydocs (although as always one can ignore certain flags).

I guess it also depends on what the purpose of this repository is. If the aim is to provide a python project base for all, then definitely the "fully featured" option needs to provide something that is "comfortable" to use and will not drive users crazy because they don't manage to commit anything without tons of errors. In that sense, mypy is already a big one to digest! So I think the current state of the "fully featured" is great!

In our (group) case, I feel that we needed to have high quality code documentation (or at minima complete) because deep-learning pipelines can become quite complex (and maybe our coding is not always the most intuitive...). There, having the strict numpydocs really helped. We had a tendency to simply put a minimum docstring comment, which became a problem when the code base grew.

Finally, if we were to add numpydocs here, you are right, I should have added a jinja if-clause to remove pydocstyle from the ruff options (and related flags).

All in all, if the addition of a complex customization interaction is done solely for a few of my projects, I am not sure that it is worth given the aim of this project!

That being said, I love ruff but the whole pydocstyle through ruff feels a bit clunky and unintuitive to me. But I guess I should just spend more time playing around with it and maybe try to reproduce the behaviour I expect! 😄

tlambert03 commented 1 year ago

I love ruff but the whole pydocstyle through ruff feels a bit clunky and unintuitive to me.

absolutely agree. for me, it's https://github.com/astral-sh/ruff/issues/2606 that's holding this up. We should just be able to say numpy + D417 ... (and, I know, even that doesn't fix the problem... so I too just kinda keep waiting for ruff docstyle to evolve)

I guess it also depends on what the purpose of this repository is.

yeah, I dunno. I'd like it to be a general best practice kinda thing... which of course necessarily implies a degree of opinion. There's no right answer, but I think that our three levels (full, minimal, custom) still allow for this very valid use case to be captured. I would definitely be supportive of adding it as an option to the customized build, but probably off by default even in the full case.

for what it's worth, I've decided to start maintaining a fork of this repo for myself (to convert from my previous cookiecutter one). I made https://github.com/tlambert03/pyrepo... made a few changes to the defaults in the copier.yml (so I don't need to enter my name for example). My intention is to keep merging the main branch of this repo into my personal repo, so I can effectively use this repo with my own personal prefs. If you added the numpydoc option to the customized version of this template, you could do a similar thing for your own group (making it on by default)?