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

Use functools.wraps instead of decorator #766

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Pliner
Copy link
Contributor

@Pliner Pliner commented Feb 8, 2022

Just a small cleanup: only 3.9+ is supported, so it looks fine to switch to functools.wraps.


return decorate(f, wrapped)
return f(*args, **kwargs)
return wrapped # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there are two options for now: ignore or cast.

PS In python 3.10 it was resolved via https://docs.python.org/3/library/typing.html#typing.ParamSpec.

@Pliner
Copy link
Contributor Author

Pliner commented Feb 8, 2022

@csmarchbanks Could you look on it please? Thanks.

@@ -54,7 +54,7 @@ def f(r):
else:
raise TypeError

self.assertEqual((["r"], None, None, None), getargspec(f))
self.assertEqual("(r)", str(inspect.signature(f)))
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'm not sure these checks are relevant now: it could be worth ditching them.

@Pliner Pliner force-pushed the use-functools-wraps-instead-of-decorator branch from 551660d to fe2a233 Compare February 8, 2022 15:20
Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Hmm, I am not super familiar with functools.wraps but I see some disagreement around if it actually preserves the signature, for example: https://stackoverflow.com/questions/13907644/decorator-module-vs-functools-wraps/55363013#55363013. Are there some recent posts around functools.wraps for 3.6+ that fully preserve the signature?

@Pliner Pliner force-pushed the use-functools-wraps-instead-of-decorator branch from 938b988 to d357585 Compare December 11, 2024 01:23
Signed-off-by: Yury Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
python/mypy#1927
Signed-off-by: Yury Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
Signed-off-by: Yury Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
Signed-off-by: Yury Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
Signed-off-by: Yury Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
Signed-off-by: Iurii Pliner <[email protected]>
@Pliner Pliner force-pushed the use-functools-wraps-instead-of-decorator branch from 29afda4 to d9aae14 Compare December 11, 2024 01:30
@Pliner Pliner force-pushed the use-functools-wraps-instead-of-decorator branch from 53b6afb to b631c0e Compare December 11, 2024 18:45
@Pliner
Copy link
Contributor Author

Pliner commented Dec 11, 2024

Hmm, I am not super familiar with functools.wraps but I see some disagreement around if it actually preserves the signature, for example: https://stackoverflow.com/questions/13907644/decorator-module-vs-functools-wraps/55363013#55363013. Are there some recent posts around functools.wraps for 3.6+ that fully preserve the signature?

Hey @csmarchbanks,

Sorry for the delayed for 2 years response.

I have investigated the mentioned downsides of functools.wraps.

The wrapper code can not easily access an argument using its name, from the received *args, **kwargs. Indeed one would have to handle all cases (positional, keyword, default)

It doesn't seem to be relevant for this library, because they are passed as it is in all usecases.

The wrapper code will execute even when the provided arguments are invalid.

It is true and might be relevant, however:

  1. It doesn't look like too bad (in my humble opinion ofc) for the use cases inside the library (btw, this fails only at runtime).
  2. A better way to catch this kind of errors might be to annotate decorators properly with ParamSpec so that incorrectly passed arguments will be detected by a type checker.

So, as part of this PR I have added a proper typing for decorators. Unfortunately,
ParamSpec is only availably from 3.10+, so I had to depend on typing_extensions for 3.9. For example, aiohttp depends on it as well (https://github.com/aio-libs/aiohttp/blob/master/requirements/base.txt#L41).

PS I don't know your opinion about this additional dependency, but it might be reasonable for the benefits we will get from the proper annotations, considering this extra dependency is only for 3.9.

@Pliner Pliner requested a review from csmarchbanks December 11, 2024 18:53
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.

2 participants