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

Deprecated getargspec #415

Closed
wants to merge 5 commits into
base: dev
from

Conversation

Projects
None yet
3 participants
@iiogmgo
Contributor

iiogmgo commented Mar 11, 2016

I fixed getargspec to signature because DeprecationWarning is occurred in Python 3.5.

@sloria

This comment has been minimized.

Member

sloria commented Mar 12, 2016

Thanks for the PR. signature doesn't exist in Py2, so the builds are failing. Can you please fix this up to be py2/3-compatible?

@sloria

This comment has been minimized.

Member

sloria commented Mar 14, 2016

This also introduces a backwards-incompatible change in how method signatures are handled. Currently, the following test passes:

    class F3(object):
        def __call__(self, foo, bar):
            pass
    f3 = F3()

    for func in [f1, f2, f3]:
        assert utils.get_func_args(func) == ['self', 'foo', 'bar']

Using inspect.signature will make this test fail.

Not sure what the best solution is. This will require more thought.

@sloria sloria added this to the 3.0 milestone May 22, 2016

@deckar01

This comment has been minimized.

Member

deckar01 commented May 25, 2016

I looked into supporting Py2/3 compatibility. It is far from trivial.

Django developer group discussing the deprecation
I ended up abstracting our usage of inspect.getargspec() into some utility methods in a django.utils.inspect module. Each method has a separate branch for Python 2 and Python 3. It's not ideal, but I think it's preferable to adding a short-lived dependency or vendored copy [...]

AFAIK marshamallow doesn't have any hard dependencies, so adding a back port like funcsigs, is probably not desirable.

I wanted to replicate the behavior of getargspec() with signature() to provide backwards compatibility, but I am afraid it is not possible due to the documented behavior of signature()

PEP 362 > Implementation
If the object is a bound method, construct and return a new Signature object, with its first parameter (usually self or cls ) removed.

Although the getmembers() method allows getting the value of bound arguments like self, it does not expose the original name the parameter was given. This makes it impossible to replicate the behavior of getargspec() when a signature like def __call__(me, arg1, arg2) renames the self parameter.

Replicating the behavior of signature() with getargspec() should be possible, but the solution in the implementation in the Django PR seems over simplified. The self arg is not always present, which is a case they don't seem to consider.

Looking at the source of funcsigs which replicates removing bound methods like self, reveals it is classifying the kind of the parameters.

kind = params[0].kind
if kind in (_POSITIONAL_OR_KEYWORD, _POSITIONAL_ONLY):
    # Drop first parameter:
    # '(p1, p2[, ...])' -> '(p2[, ...])'
    params = params[1:]

funcsigs/init.py#L73

In summary, providing compatibility between Python 2 and 3 requires back porting a non-trivial amount of the signature implementation for Python 2.

@deckar01

This comment has been minimized.

Member

deckar01 commented May 27, 2016

I have a reasonably simple fallback for Python 2 working. I opened a PR against iiogmgo/deprecated-getargspec.

This is basically the same strategy used in the Django PR, but with additional handling for bound methods.
http://stackoverflow.com/questions/53225/how-do-you-check-whether-a-python-method-is-bound-or-not

Using inspect.signature will make this test fail.

It really should. Consider how this utility is being used currently.

https://github.com/marshmallow-code/marshmallow/blob/dev/marshmallow/fields.py#L1203

That logic is currently invalid for bound methods, because including self in the arg count would make _call_or_raise think that the callable can handle a context for the second parameter, when in reality it can only be called with one parameter. There does not seem to be any tests where a Function field is constructed with a bound serializer or deserializer that accept a single unbound parameter, but they would currently fail.

I added a test to the PR for providing a bound method with one unbound parameter as the serializer for a Function field. On dev it fails with the exception No context available for Function field 'is_collab', but now that bound parameters are ignored it passes.

@sloria

This comment has been minimized.

Member

sloria commented May 29, 2016

Thanks @deckar01 for looking into this. I'm thinking that the best solution might be to change the behavior of get_func_args to match the behavior of inspect.signature, i.e. omit self for bound methods.

This would happen in marshmallow 3.0, since it's a breaking change.

@deckar01 deckar01 referenced this pull request Jun 22, 2016

Closed

Deprecated getargspec #478

@deckar01

This comment has been minimized.

Member

deckar01 commented Jun 22, 2016

I went ahead and opened #478 since merging my PR into this PR may not be a viable approach.

@deckar01 deckar01 referenced this pull request Jun 23, 2016

Merged

Deprecated getargspec #479

@sloria sloria closed this Jun 24, 2016

@sloria sloria modified the milestones: 3.0, 3.0a Feb 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment