GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.03k stars 231 forks source link

un-necessary "6-duplicated" _unpack_self function #240

Closed gst closed 11 months ago

gst commented 1 year ago

https://github.com/GrahamDumpleton/wrapt/blob/980e4b451c2b3cdef1d32c9ae10d97894dd3735e/src/wrapt/wrappers.py#L478

        def _unpack_self(self, *args):
            return self, args

repeated 6 times. and could be, simply, short-cutted to:

self, *args = args

in the 6 cases where it's used.

EDIT: though, hmm, then the args becomes a list.

maybe the _unpack_self function helper could simply be put in module top level, so to keep the tuple instead, and simply remove the duplications of it.

GrahamDumpleton commented 1 year ago

In most cases it wouldn't matter if it is a list. There is at least one case though where resulting args is added to an existing tuple using the + operator, which would then fail if is a list.

Except for dealing with that case the suggestions seems reasonable and worth looking at.

GrahamDumpleton commented 1 year ago

This can only be changed when drop Python 2.7 support. Since GitHub actions finally no longer provides ability to test with Python 2.7, that time may finally be now. :-)

GrahamDumpleton commented 1 year ago

Making the change to use self, *args = args will break stuff as things being called where that is passed would usually assume it would be a tuple, as that is what *args usually results in. So would have to convert back to tuples, making it more complicated than simple assignment.

gst commented 1 year ago

maybe the _unpack_self function helper could simply be put in module top level, so to keep/still have the tuple instead, and simply remove the duplications of this function thus ?

mentalisttraceur commented 11 months ago

@gst it also needs to be a function because tuple unpacking raises a different error type if there's not enough arguments.

When a mandatory argument is missing, Python raises a TypeError. When a tuple unpacking fails, it raises ValueError.

mentalisttraceur commented 11 months ago

The duplications might be my fault - when I proposed this positional-only-self backport, the unpack-self function was always nested in its caller, for a couple reasons:

  1. This lets the unpack-self function have the same name as the function containing it, which in turn lets the TypeError's message also be identical to the native error.

  2. This lets the traceback be as-close-as-it-can-be to native, not just in function name but also in line number (because the traceback will have the unpack-self function as the last entry).

But either I didn't convey the same-name benefit clearly enough, or Graham decided it was more valuable to have a clearer name in the source than the same name in the message+traceback (and while that's not what I would do, maybe he's righter/wiser and I don't blame him - immitating the native error beyond type gives much less benefit, and a good argument could be made that a self-explanatory name is worth more).

GrahamDumpleton commented 11 months ago

FWIW, right now am taking attitude that if it isn't broken then more inclined to leave it how it is. :-)

The duplication is because for very little helpers like that I tend to favour just copy pasting everywhere so the function is self contained as much as possible, rather than separating out what is a quite trivial function.

So the question is, is this causing incorrect or confusing behaviour. I don't fully understand what you are talking about when you say:

But either I didn't convey the same-name benefit clearly enough, or Graham decided it was more valuable to have a clearer name in the source than the same name in the message+traceback (and while that's not what I would do, maybe he's righter/wiser and I don't blame him - immitating the native error beyond type gives much less benefit, and a good argument could be made that a self-explanatory name is worth more).

Don't assume I did anything for sane reasons.