Skip to content
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

Fix #2826 #3092

Merged
merged 4 commits into from Mar 14, 2019

Conversation

Projects
None yet
4 participants
@zhelezov
Copy link
Contributor

commented Dec 10, 2018

Test for major Python version and use inspect.getargspec() or
inspect.getfullargspec() respectively to silence deprecation warnings in
Python 3 as discussed in #2826

Fix #2826
Test for major Python version and use inspect.getargspec() or
inspect.getfullargspec() respectively to silence deprecation warnings in
Python 3
@zhelezov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Having another look at this I don't like the fact that argspec is of a different type in the two branches: 4-tuple of type ArgSpec(args, varargs, keywords, defaults) in the PY2 case, and 7-tuple of type FullArgSpec(args, varargs, varkw, defaults, kwonlyargs, kwonlydefaults, annotations) in the PY3 branch. The above proposed simple solution is alright because we only ever use the first member argspec.args but it is a possible source of future confusion should one try to use the third member which is not accessible by the same attribute: argspec.keywords vs argspec.varkw.

To make the tuple identical in both cases I propose two solutions.

The simple one:

diff --git a/beets/plugins.py b/beets/plugins.py
index 69784d26..9a984cf5 100644
--- a/beets/plugins.py
+++ b/beets/plugins.py
@@ -130,7 +130,7 @@ class BeetsPlugin(object):
         if six.PY2:
             argspec = inspect.getargspec(func)
         else:
-            argspec = inspect.getfullargspec(func)
+            argspec = inspect.ArgSpec(*inspect.getfullargspec(func)[:4])

         @wraps(func)
         def wrapper(*args, **kwargs):

What bugs me about this is that if inspect.getargspec() is removed from Python 3.8, so would most likely be inspect.ArgSpec and this will soon break.

The second solution is to replicate inspect.ArgSpec explicitly, sth like this:

diff --git a/beets/plugins.py b/beets/plugins.py
index 69784d26..12d9dde5 100644
--- a/beets/plugins.py
+++ b/beets/plugins.py
@@ -21,6 +21,7 @@ import inspect
 import traceback
 import re
 from collections import defaultdict
+from collections import namedtuple
 from functools import wraps


@@ -130,7 +131,9 @@ class BeetsPlugin(object):
         if six.PY2:
             argspec = inspect.getargspec(func)
         else:
-            argspec = inspect.getfullargspec(func)
+            ArgSpec = namedtuple('ArgSpec', 'args varargs keywords defaults',
+                                 defaults=([], None, None, None))
+            argspec = ArgSpec(*inspect.getfullargspec(func)[:4])

         @wraps(func)
         def wrapper(*args, **kwargs):

What are your thoughts about my concerns? Leave it like it is, maybe just add a comment with a warning, or commit the second proposed solution which looks more robust, if a bit clunkier, to me?

@zhelezov

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

A third variant, stolen more or less from Django's code base, is to use inspect.signature which is the recommended aproach in Python 3. There was a proposal from Python devs to get rid from both getargspec and getfullargspec even if for the moment they seem to have backed off from the decision to deprecate the latter. In that case we can introduce the wrapper in say beets/util/inspect.py:

from __future__ import absolute_import

import inspect
from collections import namedtuple

from six import PY2


def getargspec(func):
    if PY2:
        return inspect.getargspec(func)

    ArgSpec = namedtuple('ArgSpec', 'args varargs keywords defaults')

    sig = inspect.signature(func)
    args = [
        p.name for p in sig.parameters.values()
        if p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
    ]
    varargs = [
        p.name for p in sig.parameters.values()
        if p.kind == inspect.Parameter.VAR_POSITIONAL
    ]
    varargs = varargs[0] if varargs else None
    varkw = [
        p.name for p in sig.parameters.values()
        if p.kind == inspect.Parameter.VAR_KEYWORD
    ]
    varkw = varkw[0] if varkw else None
    defaults = tuple(p.default for p in sig.parameters.values()
                     if p.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD
                     and p.default is not p.empty) or None

    return ArgSpec(args, varargs, varkw, defaults)

And import it elsewhere in place of inspect from stdlib. In the case of beets/plugins.py the sole change needed would be the import:

diff --git a/beets/plugins.py b/beets/plugins.py
index 6dec7ef2..56bb07cb 100644
--- a/beets/plugins.py
+++ b/beets/plugins.py
@@ -17,7 +17,6 @@

 from __future__ import division, absolute_import, print_function

-import inspect
 import traceback
 import re
 from collections import defaultdict
@@ -27,6 +26,7 @@ from functools import wraps
 import beets
 from beets import logging
 from beets import mediafile
+from beets.util import inspect
 import six

 PLUGIN_NAMESPACE = 'beetsplug'
@bb010g

This comment has been minimized.

Copy link

commented Jan 12, 2019

The wrapper option looks like the best path forwards to me.

@ssssam

This comment has been minimized.

Copy link

commented Mar 4, 2019

I've tested this PR and it fixes the deprecation warnings issue for me. Shall we merge it? Perhaps with a link back to the comment above documenting the potential gotcha for future devs?

@zhelezov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

I am sorry about the delay, almost forgot about this issue actually. I pushed the wrapper option from my last post as suggested. Feel free to do as you please. Cheers.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

This does look like quite a complete solution! The only thing is: when we (hopefully before too long) transition to Python 3 exclusively, I predict that we may not need perfect emulation of getargspec. We only need to pull out a few attributes for our AST-processing purposes, which makes this (very clean!) drop-in replacement somewhat overkill. But it's probably fine for now.

@zhelezov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Thought about the same thing. That's why I isolated the workaround in a separate file which can simply be removed after the transition of the code base to Python 3. I don't see a cleaner solution having in mind backward compatibility.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Indeed; seems like a great idea for this to get isolated in its own separate file. The thing I was bringing up, however, is that we're currently emulating a Python 2 API on Python 3, not the other way around. So when we make the transition, we won't be able to just remove the file---we'll be able to remove the if PY2: early abort, but more refactoring will be required to use the new API "directly."

Show resolved Hide resolved beets/util/inspect.py Outdated

@sampsyo sampsyo merged commit 8ae2b47 into beetbox:master Mar 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Mar 14, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Fantastic; thank you!

@zhelezov zhelezov deleted the zhelezov:2826-deprecation-warnings branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.