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

Exempt properties from D401 #546

Merged
merged 15 commits into from
Aug 15, 2021
Merged

Exempt properties from D401 #546

merged 15 commits into from
Aug 15, 2021

Conversation

TomFryers
Copy link
Contributor

This addresses issue #531. It excludes any method decorated with a decorator with "property" in the name in order to catch cached_property and other custom property types. Maybe there’s a fancier way of doing this, but I don’t know what it is.

def is_property(self):
"""Return True iff the method decorated with property."""
for decorator in self.decorators:
if "property" in decorator.name:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure if I create a decorator named myproperty this will disable D401 which is far from ideal. Why not if decorator.name == "property":?

@@ -218,6 +218,14 @@ def is_overload(self):
return True
return False

@property
def is_property(self):
"""Return True iff the method decorated with property."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says iff which tends to only mean "if and only if" but that's not actually true here because anything with the string "property" in the name will return True, not just the @property built-in decorator

@sigmavirus24
Copy link
Member

So if you want to catch both @property and @cached_property then I'd suggest the best way is to add a flag called --property-function-names (or something like that) and then use that to do if decorator.name in options.property_fn_names

@@ -218,6 +218,13 @@ def is_overload(self):
return True
return False

def is_property(self, decorators):
"""Return True iff the method decorated with property."""
for decorator in self.decorators:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding what the difference is between self.decorators and the argument decorators. Can these names be more descriptive?

Also couldn't this be something like

return any(decorator.name in decorators for decorator in self.decorators)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve improved this. I also changed the above is_overload to match.

sambhav
sambhav previously approved these changes Aug 15, 2021
docs/release_notes.rst Outdated Show resolved Hide resolved
@sambhav
Copy link
Member

sambhav commented Aug 15, 2021

@TomFryers sorry for the delay. Thank you very much for this contribution :)!

Also thanks @sigmavirus24 for reviewing :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants