Skip to content

Use getfullargspec under the scenes for py3 to stop DeprecationWarning #2864

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

Merged

Conversation

andreip
Copy link
Contributor

@andreip andreip commented Jul 29, 2017

fixes #2862

I also ran tox for the tests, but there are slight differences for the packages used inside the library, so in tests/requirements.txt:

  • netlib can be easily updated to 0.17 to support both py2 and py3
  • mitmproxy on the other hand can't w/o some modifications; its version 0.18+ that supports both py3 and py2 doesn't contain libmproxy that's used in tests/test_proxy_connect.py. So yea, related to enable test_proxy_connect in Python 3 #2545 actually, so nevermind.

Other than that issue, everything worked fine, no errors for py2 nor py3.

Also I tested the branch in our project and the DeprecationWarning has disappeared now :).

@codecov-io
Copy link

codecov-io commented Jul 29, 2017

Codecov Report

Merging #2864 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2864      +/-   ##
=========================================
+ Coverage   84.69%   84.7%   +<.01%     
=========================================
  Files         164     164              
  Lines        9189    9192       +3     
  Branches     1369    1370       +1     
=========================================
+ Hits         7783    7786       +3     
  Misses       1154    1154              
  Partials      252     252
Impacted Files Coverage Δ
scrapy/utils/python.py 84.35% <100%> (+0.26%) ⬆️

def getargspec(func):
full_argspec = list(inspect.getfullargspec(func))
return inspect.ArgSpec(*full_argspec[:4])
inspect.getargspec = getargspec
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about creating an utility function and using it, without patching inspect module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any name suggestion? getargspec_py23?

dtran320 pushed a commit to UpbeatPR/scrapy that referenced this pull request Jul 31, 2017
>>> getargspec_py23(f)
ArgSpec(args=['a', 'b'], varargs='ar', keywords='kw', defaults=(2,))
"""
if six.PY3:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall, @kmike prefers if not six.PY2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go the other way around:

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

return inspect.ArgSpec(*inspect.getfullargspec(func)[:4])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I would make this function private, so we reserve the right to drop it if not needed by other utils in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, makes sense to do that as long as usage of inspect.getargspec stays within the utils.python module, as is now. If at some point inspect.getargspec gets used outside, it probably makes sense to do from scrapy.utils.python import getargspec instead of from inspect import getargspec to be able to use that for both py2 and py3 without triggering deprecation warning.

@dangra dangra merged commit 6e6b5cc into scrapy:master Aug 1, 2017
@dangra
Copy link
Member

dangra commented Aug 1, 2017

thanks!

@kmike kmike added this to the v1.5 milestone Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeprecationWarning: inspect.getargspec()
5 participants