PyCQA / pydocstyle

docstring style checker
http://pydocstyle.org
MIT License
1.11k stars 188 forks source link

Disable D102 for functions decorated with @<property>.setter, @<property>.deleter #566

Open lukelbd opened 2 years ago

lukelbd commented 2 years ago

Follow-up from discussion in #68. IMO functions decorated with @property should be excluded from imperative mood checking.

Given this example:

@property
def quantity(self):
    """The data values of this `~xarray.DataArray` as a `pint.Quantity`."""

The resulting error is this:

D401: First line should be in imperative mood; try rephrasing

Imperative mood checking is extremely useful, but I currently ignore all D401 errors because of this issue.

lordmauve commented 2 years ago

I'll elaborate on what I said in #68.

You can just write "Get":

@property
def quantity(self):
    """Get the data values of this `~xarray.DataArray` as a `pint.Quantity`."""

If you don't do this then you don't have a docstring to write for a property's setter or deleter, or they'll mismatch in style.

If you don't write a docstring for the setter you'll get a D102. But also if you're in the habit of skipping docstrings in certain situations you can miss the opportunity to describe useful insights. "Set the data values" is not very useful but "Set the data values and invalidate caches" is.

lukelbd commented 2 years ago

You can just write "Get"

To me the addition of an action verb implies quantity is a class method rather than attribute, i.e. it needs to be called with quantity(). Could come across as confusing for new users.

If you don't write a docstring for the setter you'll get a D102.

Maybe that should be disabled as well... as far as I understand, in most (all?) use cases, the property setter and deleter docstrings are hidden from users. For example, with this code:

class A(object):
    @property
    def a(self):
        """Hello world!"""
        return 1
    @a.setter
    def a(self, value):
        """Missing docstring!"""
        raise NotImplementedError

I get the following result after calling help(A):

Help on class A in module __main__:

class A
 |  Data descriptors defined here:
 |
 |  a
 |      Hello world!

If D102 is meant only to prevent exposing undocumented public methods to users, perhaps it is unnecessary here... since Missing docstring! it is not exposed to users, it is effectively "private" and should not require a docstring for consistency with the rest of the style. Although this would be a very specialized exception.

lordmauve commented 2 years ago

the addition of an action verb implies quantity is a class method rather than attribute

It is a method. The thing that is a class attribute is the descriptor created by property().

I think there's a philosophical difference here: are docstrings for external consumers of an API (docs/introspection) or for readers of the source code? For me it's 40% docs, 60% readers. One reason I feel like this is that I read a lot of source code with inadequate commenting, while Sphinx gives me a lot of flexibility to shape how I document things for users of an API.

lukelbd commented 2 years ago

I'd argue that in general, docstrings are for users and comments are for readers of code. Thus, the docstring in a @property getter should be thought of as a "thing that will document the resulting data descriptor" rather than a "thing documenting the internal, hidden getter method".

This is a very subjective take, but the current pydocstyle seems to implicitly promote this philosophy (error codes for missing docstrings on "public" objects only). So, I'd argue this feature request is at least consistent with precedent.

merwok commented 2 years ago

PEP 257 does not mention properties, but a quick look at some modules in the standard library agrees with Luke: properties are named with nouns, and documented like attributes, not like methods.

GameDungeon commented 2 years ago

Also agreed here, properties should be named with nouns.

e-gebes commented 2 years ago

Also agree, properties should be documented like attributes. If a property is expensive or complicated, rather a normal method with a normal docstring should be used, instead of documenting an attribute like a function.

Isn't this request solved already with https://github.com/PyCQA/pydocstyle/issues/531 ?

lukelbd commented 2 years ago

Guess so! Looks like there hasn't been a release since #546 was merged, and somehow missed #531 and #546 when googling the issue. Closing this as resolved / duplicate.

lukelbd commented 2 years ago

On second thought, I think I'll keep this open to discuss the closely related issue mentioned in my comment above: D102 should be disabled for functions decorated as property setters and deleters, since their docstrings are hidden from users.