-
Notifications
You must be signed in to change notification settings - Fork 842
Use functools.wraps instead of decorator#766
Use functools.wraps instead of decorator #766Pliner wants to merge 7 commits intoprometheus:master from
Conversation
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.
Pliner
commented
Feb 8, 2022
@csmarchbanks Could you look on it please? Thanks.
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
@csmarchbanks
csmarchbanks
left a comment
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 <yury.pliner@gmail.com> Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
python/mypy#1927 Signed-off-by: Yury Pliner <yury.pliner@gmail.com> Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
Signed-off-by: Yury Pliner <yury.pliner@gmail.com> Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
Signed-off-by: Yury Pliner <yury.pliner@gmail.com> Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
Signed-off-by: Yury Pliner <yury.pliner@gmail.com> Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
29afda4 to
d9aae14
Compare
Signed-off-by: Iurii Pliner <yury.pliner@gmail.com>
53b6afb to
b631c0e
Compare
Hmm, I am not super familiar with
functools.wrapsbut 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 aroundfunctools.wrapsfor 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:
- 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).
- A better way to catch this kind of errors might be to annotate decorators properly with
ParamSpecso 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
commented
May 25, 2025
Hi @csmarchbanks,
I am sorry for pinging you... Could you have a look on my last reply please? 🙏🏻
Uh oh!
There was an error while loading. Please reload this page.
Just a small cleanup: only 3.9+ is supported, so it looks fine to switch to
functools.wraps.