On 08/07 10:49, Joao S. O. Bueno wrote: > "3" would "break stuff around". > I'd be glad if "2" worked - but that have to be well documented and > easy to see.
I've update the PR with improved documentation and comments to make it easier to see. At this point I'd appreciate if I get some reviews to improve the patch, thanks! > > "1" is basically a workaround the current behavior, and people who > care for it will have to continue doing that until Python 3.10 is > widespread - but ultimately it basically requires that everyone doing that > re-implements whatever logic you have in that PR for "2" (thus defeating > the very purpose of "@wraps" - I'd rather rebind wrapper.__name__ and > __wrapped__ manually) > > TL;DR: I agree that "2" is the way to go. > > > On Fri, 7 Aug 2020 at 09:14, David Caro <da...@dcaro.es> wrote: > > > > > Hi! > > > > Some time ago I opened a bug and sent a patch, but I realize that I > > should have started a chat here first, better late than never. > > > > The default behavior of the `functools.wraps` decorator, is to copy over > > the `__annotations__` from the wrapped function to the wrapper function. > > > > This is ok if the wrapper function has the same signature that the > > wrapped one (that I guess was the main goal behind it, just a simple > > wrapper). > > > > The issue comes when the wrapper function has a different signature than > > the wrapped, for example, the `contextlib.contextmanager` decorator, > > changes the return value, returning a `_GeneratorContextManager`: > > > > ``` > > def contextmanager(func): > > """... > > """ > > @wraps(func) > > def helper(*args, **kwds): > > return _GeneratorContextManager(func, args, kwds) > > return helper > > ``` > > > > but as it uses `wraps`, the `__annotations__` will be copied over, and > > the new function will have an incorrect return type there. > > > > > > In order to improve this, I have some proposals: > > > > 1. Keep the default behavior of `wraps`, but change the usage around the > > library to not copy over the `__annotations__` in places (like > > `contextmanager`) where the types change. > > > > Advantages are that `wraps` keeps being backwards compatible, though the > > change in `contextmanager` might force some people to "fix" their > > annotations, I would consider that a "bugfix", more than a behavior change. > > > > The disadvantage is that you have to know that `wraps` will overwrite > > the wrapped function annotations by default, and I think that it's > > broadly used when creating decorators that have different signature/types > > than > > the wrapped function, so people will have to explicitly change their > > code. > > > > > > 2. Change `wraps` so if there's any type of annotation in the wrapper > > function, will not overwrite it. > > This is what I did in the PR (though I'm not convinced). > > > > Advantages are that only people that took the time to annotate the > > wrapper function will see the change, and that will be the change they > > expect (that's my guess though). > > It's a bit smart with it, so if you don't specify a return type, will > > get it from the wrapped function, or if you don't specify types for the > > arguments will get them from the wrapped function, filling only the gap > > that was not specified. > > For everyone else, `wraps` is backwards compatible. > > > > Disadvantages, it's a bit more convoluted as it requires some logic to > > detect what annotations are defined in the wrapper and which ones in the > > wrapped and merge them. > > > > > > 3. Change the default behavior of `wraps` and don't overwrite the > > `__annotations__` property. This is non-backwards compatible, but imo > > the simplest. > > > > Advantages are that it's very simple, and you'll end up with valid, > > straight-forward `__annotations__` in the wrapped function (that is, > > whatever you declare when writing the wrapped function). > > > > Disadvantages, if the goal of the `wraps` decorator was as a helper when > > wrapping functions in a simple manner (not changing the signature), then > > this becomes a bit more inconvenient, as it will not copy the > > `__annotations__` as it was doing by default. It also changes the > > current default behavior, so it will potentially affect everyone that > > uses `wraps` currently. > > > > > > Ideas? > > > > > > Thanks! > > > > -- > > David Caro > > _______________________________________________ > > Python-ideas mailing list -- python-ideas@python.org > > To unsubscribe send an email to python-ideas-le...@python.org > > https://mail.python.org/mailman3/lists/python-ideas.python.org/ > > Message archived at > > https://mail.python.org/archives/list/python-ideas@python.org/message/LFF3EXFPZG3FMXY4AXORU6SXXKBPSZOY/ > > Code of Conduct: http://python.org/psf/codeofconduct/ > > -- David Caro _______________________________________________ Python-ideas mailing list -- python-ideas@python.org To unsubscribe send an email to python-ideas-le...@python.org https://mail.python.org/mailman3/lists/python-ideas.python.org/ Message archived at https://mail.python.org/archives/list/python-ideas@python.org/message/BIHX4FA6QB3QSSARNRT7VUKGOBE23IVD/ Code of Conduct: http://python.org/psf/codeofconduct/
