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

Proposed fix for issue #358 #359

Merged
merged 5 commits into from Nov 6, 2022
Merged

Conversation

eachimei
Copy link
Contributor

@eachimei eachimei commented Aug 3, 2022

Switch to inspect.signature instead of inspect.getfullargspec to properly support wrapper chains (decorated functions) - see details in issue #358 .

In the proposed change I tried to do the minimal required changes to varnames.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Lovely start, please see my comments for the self handling

@@ -245,11 +245,29 @@ def varnames(func: object) -> Tuple[Tuple[str, ...], Tuple[str, ...]]:
return (), ()

try: # func MUST be a function or method here or we won't parse any args
spec = inspect.getfullargspec(func)
sig = inspect.signature(func.__func__ if inspect.ismethod(func) else func)
Copy link
Member

Choose a reason for hiding this comment

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

If a method signature conveniently skips self, then we could drop the pypy vs cython drop self hack just below by always using func here

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 thought about that, and the challenge/problem then becomes with cases where @hookspec is used on methods or when a class object is passed into varnames (then the class __init__ is used). Meaning, in those cases func is an unbound function so signature(func) doesn't skip self. However, in the existing ("old") implementation, self is always stripped (even when func is unbound).

Copy link
Member

Choose a reason for hiding this comment

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

those are cases we might want to deprecate in a followup

  1. while signatures of Myclass.__init__ include self, signatures of a Myclass do work correct
  2. its arguably a mistake that hookspecs as classes where allowed, we should have passed in a object in any case

i'll open up issues for that

testing/test_helpers.py Show resolved Hide resolved
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

looks good, thanks

the failures seem unrelated, will fix them later today + merge this

@eachimei
Copy link
Contributor Author

eachimei commented Aug 3, 2022

That's great, thank you 😊

@nicoddemus
Copy link
Member

@eachimei can you please rebase this over main? It should run cleanly now.

@eachimei
Copy link
Contributor Author

eachimei commented Nov 6, 2022

@eachimei can you please rebase this over main? It should run cleanly now.
@nicoddemus , I rebased so we have fresh results now

@nicoddemus
Copy link
Member

nicoddemus commented Nov 6, 2022

Thanks @eachimei!

@RonnyPfannschmidt is this good to merge now?

@RonnyPfannschmidt
Copy link
Member

It seems like we didn't install pre-commit here as well, the lint fail is from that, so lint fix needed

@nicoddemus nicoddemus closed this Nov 6, 2022
@nicoddemus nicoddemus reopened this Nov 6, 2022
@nicoddemus
Copy link
Member

Enabled pre-commit for pluggy. 👍

@nicoddemus
Copy link
Member

I did fix it locally but unfortunately I cannot push to eachimei:main (permission denied), here's the diff:

diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py
index d376eca..73f0e80 100644
--- a/src/pluggy/_hooks.py
+++ b/src/pluggy/_hooks.py
@@ -244,8 +244,11 @@ def varnames(func: object) -> Tuple[Tuple[str, ...], Tuple[str, ...]]:
         except Exception:
             return (), ()

-    try:  # func MUST be a function or method here or we won't parse any args
-        sig = inspect.signature(func.__func__ if inspect.ismethod(func) else func)
+    try:
+        # func MUST be a function or method here or we won't parse any args.
+        sig = inspect.signature(
+            func.__func__ if inspect.ismethod(func) else func  # type:ignore[arg-type]
+        )
     except TypeError:
         return (), ()

diff --git a/testing/test_helpers.py b/testing/test_helpers.py
index d7ec6ce..4ddd945 100644
--- a/testing/test_helpers.py
+++ b/testing/test_helpers.py
@@ -87,12 +87,13 @@ def test_formatdef() -> None:


 def test_varnames_decorator() -> None:
-    F = TypeVar('F', bound=Callable[..., Any])
+    F = TypeVar("F", bound=Callable[..., Any])

     def my_decorator(func: F) -> F:
         @wraps(func)
         def wrapper(*args, **kwargs):
             return func(*args, **kwargs)
+
         return cast(F, wrapper)

     @my_decorator

@eachimei
Copy link
Contributor Author

eachimei commented Nov 6, 2022

@nicoddemus can you please retry? It had the "allow edit" setting set to "false", it should work now

@nicoddemus
Copy link
Member

Yeah now it works. 👍

@RonnyPfannschmidt RonnyPfannschmidt merged commit f69e314 into pytest-dev:main Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants