-
Notifications
You must be signed in to change notification settings - Fork 19
Support @property
+ @abstractmethod
without a Returns: section
#261
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
base: main
Are you sure you want to change the base?
Support @property
+ @abstractmethod
without a Returns: section
#261
Conversation
and any( | ||
( # noqa: PAR001 | ||
isinstance(_, ast.Name) | ||
and hasattr(node.decorator_list[-1], 'id') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the new test case, abstractmethod
is the last element in the decorator list and it does not (at that time) have an id
attribute, so this test failed despite there being a @property
decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given my reply below (#261 (comment)), I think we'd need to change this line into:
and hasattr(node.decorator_list[0], 'id')
Hi @barometz , thanks so much for finding this issue! I think at the time when I implemented this, I got things backwards. For example, class Foo:
@something
@property
def x(self): return "hi" is equivalent to: def x(self): return "hi"
x = property(x)
x = something(x) Then, When we put class Foo:
@property
@something
def x(self): return "hi" which is equivalent to: def x(self): return "hi"
x = something(x)
x = property(x)
|
| `DOC202` | Function/method has a return section in docstring, but there are no return statements or annotations | | ||
| `DOC203` | Return type(s) in the docstring not consistent with the return annotation | | ||
|
||
Note on `DOC201`: Methods with `@property` as its last decorator do not need to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to replace "its last decorator" with "its outer-most decorator (i.e., on the top)"
Right now, This function is also used to check for |
The DOC2xx documentation explains that
The "last decorator" restriction means this doesn't work for abstract methods, as in
.. because
@abstractmethod
can't decorate@property
(I think because the object returned byproperty()
is a native one, soabstractmethod()
can't tag it with new attributes?).I couldn't find a reason for that restriction, so this PR removes it. If there is a reason, please let me know - I'll happily put some more time into making this case work.