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

Use inspect.signature to enable Python 3 only features when defining tasks #373

Closed
wants to merge 1 commit into from

Conversation

rsxm
Copy link

@rsxm rsxm commented Jul 6, 2016

Fix #372

@rsxm
Copy link
Author

rsxm commented Jul 6, 2016

Locally all tests have passed when running "invoke test":

Ran 596 tests in 2.798 seconds
OK (10 skipped)

@rsxm rsxm closed this Jul 6, 2016
@rsxm rsxm reopened this Jul 6, 2016
@bitprophet
Copy link
Member

bitprophet commented Jul 6, 2016

Linking this to #357.

@jr-minnaar: a) thanks! b) if you can, please check the assertion made in #357 about how there might be even more to this than simple getargspec/getfullargspec - it'd be nice for this to degrade gracefully as much as possible, so if there's things even newer/"better" than getfullargspec, it'd be rad to use them if it makes sense. (So e.g. inspect.signature -> not there? getfullargspec -> also not there? getargspec)

@rsxm
Copy link
Author

rsxm commented Jul 7, 2016

Thanks, I'll have a look at #357. Do I leave this pull request open while investigating or what is the protocol?

@codecov-io
Copy link

codecov-io commented Jul 7, 2016

Codecov Report

Merging #373 into master will decrease coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
- Coverage   92.83%   92.81%   -0.03%     
==========================================
  Files          21       21              
  Lines        2149     2156       +7     
  Branches      379      382       +3     
==========================================
+ Hits         1995     2001       +6     
  Misses        113      113              
- Partials       41       42       +1
Impacted Files Coverage Δ
invoke/tasks.py 95.48% <90.9%> (-0.41%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e21055...a50329a. Read the comment docs.

@bitprophet
Copy link
Member

Leaving this open is fine, I assume you'd just push more commits once you finish looking into that stuff. Thanks!

@rsxm rsxm changed the title Use getfullargspec for Python3 tasks Allow use of function annotations when defining tasks in Python3 Aug 20, 2016
@rsxm rsxm changed the title Allow use of function annotations when defining tasks in Python3 Use inspect.signature to enable Python 3 only features when defining tasks Aug 20, 2016
@rsxm
Copy link
Author

rsxm commented Aug 20, 2016

I've updated the pull request to now use inspect.signature instead of inspect.getargspec.

I've also addressed one of the TODO's that related to the argspec method. Let me know if I should rather take that out of this pull request.

@bitprophet
Copy link
Member

If you have a sec can you write out a list of which Python versions have which of getargspec, getfullargspec, signature, etc? I'd like to make sure we're not leaving e.g. Python 3.3 or 2.6 or whatever, out in the cold here.

If "just" both of getargspec and signature covers all of 2.6, 2.7, 3.3, 3.4, 3.5+...that'd be fine :) (We no longer bother with 3.2 so maybe that's where some of the earlier confusion came in.)

@bitprophet bitprophet added this to the 0.14 milestone Aug 22, 2016
@rsxm
Copy link
Author

rsxm commented Aug 22, 2016

If "just" both of getargspec and signature covers all of 2.6, 2.7, 3.3, 3.4, 3.5+...that'd be fine :) (We no longer bother with 3.2 so maybe that's where some of the earlier confusion came in.)

That indeed seems to be the case:

Python 2 getargspec getfullargspec signature
2.6 ✅ Changed ❌ No ❌ No
2.7 ✅ Yes ❌ No ❌ No
Python 3 getargspec getfullargspec signature
3.2 ⚠️ Deprecated ✅ Yes ❌ No
3.3 ⚠️ Deprecated ✅ Yes ✅ New
3.4 ⚠️ Deprecated ✅ Changed ✅ Yes
3.5 ⚠️ Deprecated ⚠️ Deprecated ✅ Yes
3.6 ⚠️ Deprecated ⚠️ Deprecated ✅ Yes

@bitprophet
Copy link
Member

Awesomesauce, thanks! Also excellent use of emoji ❤️

@rsxm rsxm force-pushed the master branch 2 times, most recently from 872e535 to a50329a Compare March 10, 2017 18:28
@rsxm
Copy link
Author

rsxm commented Mar 10, 2017

Hi @bitprophet

I see you've moved this pull request to the 0.16 milestone. I've rebased and rerun tests. Also removed some extra changes not related to this issue.

Looking forward to the next release :-)

Use inspect.signature instead of inspect.getargspect when
using Python 3 to allow function annotations to be used in tasks.

Also fix missing context in Collection test.

Fixes pyinvoke#357

Make Travis CI flake8 exit with 0

Revert adding custom exception
@FranklinChen
Copy link

I'm sad to see this wasn't fixed for the 1.0.0 release that just came out.

@haydenflinner
Copy link
Contributor

There's actually an easier way than the PR for this commit gives, since decorator includes a similar thing to what this PR implements

commit 14c88f9b3bc7525df66a3d4ec2ad13a733e08bf1
Author: Hayden Flinner <[email protected]>
Date:   Mon Nov 5 20:19:02 2018 -0500

    Fixed getfullargspec on Py2.

diff --git a/invoke/tasks.py b/invoke/tasks.py
index 7fa67025..79833b8f 100644
--- a/invoke/tasks.py
+++ b/invoke/tasks.py
@@ -8,7 +8,7 @@ from copy import deepcopy
 import inspect
 import types
 
-from .util import six
+from .util import six, getfullargspec
 
 if six.PY3:
     from itertools import zip_longest
@@ -156,7 +156,7 @@ class Task(object):
         # TODO: __call__ exhibits the 'self' arg; do we manually nix 1st result
         # in argspec, or is there a way to get the "really callable" spec?
         func = body if isinstance(body, types.FunctionType) else body.__call__
-        spec = inspect.getfullargspec(func)
+        spec = getfullargspec(func)
         arg_names = spec.args[:]
 
         # Collect into for regular args
diff --git a/invoke/util.py b/invoke/util.py
index a574580a..c70251c3 100644
--- a/invoke/util.py
+++ b/invoke/util.py
@@ -22,6 +22,7 @@ try:
     from .vendor.lexicon import Lexicon  # noqa
     from .vendor import six
     from .vendor.six.moves import reduce  # noqa
+    from .vendor.decorator import getfullargspec
 
     if six.PY3:
         from .vendor import yaml3 as yaml  # noqa

You can see it working here, I did it to use type-annotations for an invoke extension I worked on.

@dperetti
Copy link

dperetti commented Feb 3, 2020

Please 🙏

@bryzgaloff
Copy link

@bitprophet does anything stop this PR from being merged? @rsxm are you using some other workaround for that issue?

@jrabbit
Copy link

jrabbit commented Apr 7, 2020

This is a priority for me, I can review or advocate or whatever we need to do to finish this?

@rsxm
Copy link
Author

rsxm commented Apr 14, 2020

@bryzgaloff hi, I still use my patched branch with this fix. I don't update this dependency much, so haven't looked at this in a long time.

@offbyone
Copy link

offbyone commented Mar 9, 2021

I can attest to this patch fixing my encounter with #357, at least.

@paambaati
Copy link

@bitprophet I noticed that you've been committing again recently. Would you mind reviewing this PR once again and merging?

@pyinvoke pyinvoke deleted a comment from ktbarrett Jan 6, 2023
@bitprophet bitprophet closed this Jan 6, 2023
@pyinvoke pyinvoke locked and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow invoke to run tasks with type hinting