Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

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

Open
lukelbd opened this issue Jan 3, 2022 · 9 comments

Comments

@lukelbd
Copy link

lukelbd commented Jan 3, 2022

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
Copy link
Contributor

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
Copy link
Author

lukelbd commented Jan 4, 2022

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
Copy link
Contributor

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
Copy link
Author

lukelbd commented Jan 4, 2022

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
Copy link

merwok commented Jan 4, 2022

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
Copy link

Also agreed here, properties should be named with nouns.

@e-gebes
Copy link

e-gebes commented Jan 17, 2022

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 #531 ?

@lukelbd
Copy link
Author

lukelbd commented Jan 17, 2022

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 lukelbd closed this as completed Jan 17, 2022
@lukelbd
Copy link
Author

lukelbd commented Jan 17, 2022

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.

@lukelbd lukelbd reopened this Jan 17, 2022
@lukelbd lukelbd changed the title Disable D401 for functions decorated with @property Disable D102 for functions decorated with @<property>.setter, @<property>.deleter Jan 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants