-
Notifications
You must be signed in to change notification settings - Fork 127
Avoid triggering @property methods on plugins when looking for hookimpls during registration #536
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?
Changes from all commits
24895d0
aae3799
90d9f80
61a9a86
2536c62
c0789a6
bfe9f91
16a2344
4cab5d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -46,6 +46,17 @@ | |||||||
) | ||||||||
|
||||||||
|
||||||||
def _attr_is_property(obj: object, name: str) -> bool: | ||||||||
"""Check if a given attr is a @property on a module, class, or object""" | ||||||||
if inspect.ismodule(obj): | ||||||||
return False # modules can never have @property methods | ||||||||
|
||||||||
base_class = obj if inspect.isclass(obj) else type(obj) | ||||||||
if isinstance(getattr(base_class, name, None), property): | ||||||||
return True | ||||||||
return False | ||||||||
|
||||||||
|
||||||||
class PluginValidationError(Exception): | ||||||||
"""Plugin failed validation. | ||||||||
|
||||||||
|
@@ -181,7 +192,20 @@ | |||||||
customize how hook implementation are picked up. By default, returns the | ||||||||
options for items decorated with :class:`HookimplMarker`. | ||||||||
""" | ||||||||
method: object = getattr(plugin, name) | ||||||||
|
||||||||
if _attr_is_property(plugin, name): | ||||||||
# @property methods can have side effects, and are never hookimpls | ||||||||
return None | ||||||||
|
||||||||
method: object | ||||||||
try: | ||||||||
method = getattr(plugin, name) | ||||||||
except AttributeError: | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're able to cook a test for this case that would be nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see comment here: #536 (comment) |
||||||||
# AttributeError: '__signature__' attribute of 'plugin' is class-only | ||||||||
pirate marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
# can be raised when trying to access some descriptor/proxied fields | ||||||||
Comment on lines
+204
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you mean to remove the line with the exception in the suggestion? imo that's crucial to be able to grep through the codebase to find the error / understand why all this logic is here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I suggest to remove it is that the logic handles any AttributeError which may happen for other reasons. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did here but it was removed from the PR for being too pydantic-specific: https://gist.github.com/pirate/01ca7a6b41595af9a480 Afaik this case is not triggered in any typical code unless you try to use a few specific weird objects w/ descriptors (like Django/Pydantic models) as plugin namespaces. I don't necessarily want to pull in those big libraries to test this escape hatch that just avoids crashing on them, but interaction with those libraries is really the only case where this code path would come up. |
||||||||
# https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 | ||||||||
return None | ||||||||
|
||||||||
if not inspect.isroutine(method): | ||||||||
return None | ||||||||
try: | ||||||||
|
@@ -286,7 +310,18 @@ | |||||||
customize how hook specifications are picked up. By default, returns the | ||||||||
options for items decorated with :class:`HookspecMarker`. | ||||||||
""" | ||||||||
method = getattr(module_or_class, name) | ||||||||
if _attr_is_property(module_or_class, name): | ||||||||
# @property methods can have side effects, and are never hookspecs | ||||||||
return None | ||||||||
|
||||||||
method: object | ||||||||
try: | ||||||||
method = getattr(module_or_class, name) | ||||||||
except AttributeError: | ||||||||
# AttributeError: '__signature__' attribute of <m_or_c> is class-only | ||||||||
# can be raised when trying to access some descriptor/proxied fields | ||||||||
Comment on lines
+321
to
+322
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
# https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 | ||||||||
return None | ||||||||
opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) | ||||||||
return opts | ||||||||
|
||||||||
|
Uh oh!
There was an error while loading. Please reload this page.