-
Notifications
You must be signed in to change notification settings - Fork 801
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
base: master
Are you sure you want to change the base?
Use functools.wraps instead of decorator #766
Conversation
|
||
return decorate(f, wrapped) | ||
return f(*args, **kwargs) | ||
return wrapped # type: ignore |
There was a problem hiding this comment.
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.
@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))) |
There was a problem hiding this comment.
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.
551660d
to
fe2a233
Compare
There was a problem hiding this 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?
938b988
to
d357585
Compare
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]>
29afda4
to
d9aae14
Compare
Signed-off-by: Iurii Pliner <[email protected]>
53b6afb
to
b631c0e
Compare
Hey @csmarchbanks, Sorry for the delayed for 2 years response. I have investigated the mentioned downsides of
It doesn't seem to be relevant for this library, because they are passed as it is in all usecases.
It is true and might be relevant, however:
So, as part of this PR I have added a proper typing for decorators. Unfortunately, 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. |
Just a small cleanup: only 3.9+ is supported, so it looks fine to switch to
functools.wraps
.