Skip to content
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

incorrect hookimpl signature inspection for decorated functions #358

Closed
eachimei opened this issue Jul 31, 2022 · 10 comments
Closed

incorrect hookimpl signature inspection for decorated functions #358

eachimei opened this issue Jul 31, 2022 · 10 comments

Comments

@eachimei
Copy link
Contributor

eachimei commented Jul 31, 2022

Please see details at: jaraco/keyring#582 (comment)

Suggestion for pluggy._hooks:varnames: use inspect.signature(func) instead of getfullargspec since it has the ability to follow wrapper chains (and that's actually the default behavior): https://docs.python.org/3/library/inspect.html#inspect.signature

As can be seen in the mentioned comment above, the current code (pluggy 1.0.0) actually breaks/limits hook-implementations.

@RonnyPfannschmidt
Copy link
Member

the "limit" was never intentional - its primarily a artifact of python 2 support, signature should be used these days

@eachimei
Copy link
Contributor Author

eachimei commented Aug 2, 2022

0001-proposed-fix-for-issue-358.patch.txt

Attached is a fix proposal (including a matching test) as a git-patch.

@RonnyPfannschmidt
Copy link
Member

Why not open a pr?, this is absolutely horrible to work with from mobile, and it's still quite a pain if I get back to the computer

@eachimei
Copy link
Contributor Author

eachimei commented Aug 2, 2022

I'd love to and I actually tried, but seems like I don't have permission to push a branch?

@nicoddemus
Copy link
Member

@eachimei the way to contribute in GitHub in general is fork the repository, push your branch there, then open a PR. 👍

eachimei added a commit to eachimei/pluggy that referenced this issue Aug 3, 2022
jaraco added a commit to jaraco/keyring that referenced this issue Sep 17, 2022
clrpackages pushed a commit to clearlinux-pkgs/pypi-keyring that referenced this issue Sep 20, 2022
…sion 23.9.3

Dmitry Shachnev (1):
      Fix wrong import name

Jason R. Coombs (5):
      Update changelog. Ref #597.
      Add test to ensure varnames are calculated correctly. Ref #596.
      Add workaround for pytest-dev/pluggy#358. Fixes #596.
      Also use functools.wraps on the wrapper in case the hook machinery relies on the name of the function.
      Remove superfluous import by using the exception from the namespace.
@eachimei
Copy link
Contributor Author

eachimei commented Oct 11, 2022 via email

@nicoddemus
Copy link
Member

You need to create a fork, then push to your fork. From there you can open a PR to this repository.

@eachimei
Copy link
Contributor Author

eachimei commented Nov 6, 2022

@nicoddemus

You need to create a fork, then push to your fork. From there you can open a PR to this repository.

Thank you, already done: PR approved but not merged yet: #359

RonnyPfannschmidt added a commit that referenced this issue Nov 6, 2022
@zanieb
Copy link

zanieb commented Dec 30, 2022

@RonnyPfannschmidt looks like this can be closed as completed.

@RonnyPfannschmidt
Copy link
Member

indeed, thanks

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

No branches or pull requests

4 participants