inspect.getcallargs() [issue 3135]

A new proposed function for inspect; see http://bugs.python.org/issue3135

[Benjamin, 2010-03-20 03:14:31]

http://codereview.appspot.com/659041/diff/1/4
File Doc/library/inspect.rst (right):

http://codereview.appspot.com/659041/diff/1/4#newcode507
Doc/library/inspect.rst:507: .. function:: getcallargs(func[, *args][, **kwds])
I would like to see it documented that

a) getcallargs() will raise exceptions similar to the ones you would get from
invoking a function incorrectly.
b) that it handles assigning self for bound methods.

http://codereview.appspot.com/659041/diff/1/4#newcode509
Doc/library/inspect.rst:509: Get the mapping of arguments to values when calling
``func(*args, **kwds)``.
How about "Bind the *args*, and *kwds* to the argument names of *func* as if it
was called with then."?

http://codereview.appspot.com/659041/diff/1/4#newcode511
Doc/library/inspect.rst:511: A dict is returned, with keys the function argument
names (including the
This can be in the same paragraph as the first phrase.

http://codereview.appspot.com/659041/diff/1/4#newcode515
Doc/library/inspect.rst:515: >>> from inspect import getcallargs
I don't think this line is needed.

http://codereview.appspot.com/659041/diff/1/2
File Lib/inspect.py (right):

http://codereview.appspot.com/659041/diff/1/2#newcode898
Lib/inspect.py:898: assigned_tuple_params = []
How about simply keeping all assigned names in a set? This will avoid a lot of
complication.

http://codereview.appspot.com/659041/diff/1/2#newcode906
Lib/inspect.py:906: try: subvalue = next(value)
Please keep these on separate lines.

http://codereview.appspot.com/659041/diff/1/2#newcode918
Lib/inspect.py:918: #------- handle methods (bound and unbound)
--------------------------------
Remove all the "-" and all similar following comments.

http://codereview.appspot.com/659041/diff/1/2#newcode923
Lib/inspect.py:923: elif not positional or not isinstance(positional[0],
func.im_class):
Why are you typechecking this here? Why not just let the function call take care
of it?

http://codereview.appspot.com/659041/diff/1/2#newcode932
Lib/inspect.py:932: num_defaults = len(defaults or ())
Conditional "len(default) if default else 0" is more expressive here.

http://codereview.appspot.com/659041/diff/1/2#newcode934
Lib/inspect.py:934: for arg,value in zip(args,positional):
Needs a space.

http://codereview.appspot.com/659041/diff/1/2#newcode937
Lib/inspect.py:937: if num_pos > num_args:
I don't think this conditional is needed.

http://codereview.appspot.com/659041/diff/1/2#newcode946
Lib/inspect.py:946: elif num_args==0 and (num_pos or has_named):
Space between the == and operands.

http://codereview.appspot.com/659041/diff/1/2#newcode951
Lib/inspect.py:951: if isinstance(arg,str) and arg in named:
Space in function call.

http://codereview.appspot.com/659041/diff/1/2#newcode954
Lib/inspect.py:954: "argument '%s'" % (f_name,arg))
Space in tuple.

http://codereview.appspot.com/659041/diff/1/2#newcode958
Lib/inspect.py:958: for arg,value in zip(args[-num_defaults:],defaults):
Space in zip() call.

http://codereview.appspot.com/659041/diff/1/2#newcode966
Lib/inspect.py:966: unexpected = unexpected.encode('ascii','replace')
You should actually encode it with sys.getdefaultencoding() and no error
handler.

http://codereview.appspot.com/659041/diff/1/2#newcode970
Lib/inspect.py:970: unassigned = sum(1 for arg in args if not is_assigned(arg))
Just use len on a listcomp.

http://codereview.appspot.com/659041/diff/1/3
File Lib/test/test_inspect.py (right):

http://codereview.appspot.com/659041/diff/1/3#newcode558
Lib/test/test_inspect.py:558: 
Add a space before this line.

http://codereview.appspot.com/659041/diff/1/3#newcode564
Lib/test/test_inspect.py:564: 
You can split this eval() calls out into variables to avoid looking like Lisp.

http://codereview.appspot.com/659041/diff/1/3#newcode569
Lib/test/test_inspect.py:569: 
Use self.assertRaises.

http://codereview.appspot.com/659041/diff/1/3#newcode587
Lib/test/test_inspect.py:587: 
How about execing a function definition instead of a lambda?

http://codereview.appspot.com/659041/diff/1/3#newcode652
Lib/test/test_inspect.py:652: 
How about test_multiple features?

http://codereview.appspot.com/659041/diff/1/3#newcode675
Lib/test/test_inspect.py:675: 
Perhaps link to bug report?

http://codereview.appspot.com/659041/diff/1/3#newcode681
Lib/test/test_inspect.py:681: 
Space between implicit tuple args.

http://codereview.appspot.com/659041/diff/1/3#newcode708
Lib/test/test_inspect.py:708: 
Space here.

http://codereview.appspot.com/659041/diff/1/3#newcode709
Lib/test/test_inspect.py:709: 
Put "pass" on a new line.

http://codereview.appspot.com/659041/diff/1/3#newcode715
Lib/test/test_inspect.py:715: 
Space in super() call.

http://codereview.appspot.com/659041/diff/1/3#newcode719
Lib/test/test_inspect.py:719: 
Space here, too.

http://codereview.appspot.com/659041/diff/1/3#newcode724
Lib/test/test_inspect.py:724: 
Space in super call.

http://codereview.appspot.com/659041/diff/1/3#newcode728
Lib/test/test_inspect.py:728: 
Space in super call.

http://codereview.appspot.com/659041/diff/1/3#newcode732
Lib/test/test_inspect.py:732: 
self.assertFalse use case?

http://codereview.appspot.com/659041/diff/1/3#newcode734
Lib/test/test_inspect.py:734: 
Space between '+'

http://codereview.appspot.com/659041/diff/1/3#newcode738
Lib/test/test_inspect.py:738: 
Space in super call.

[George Sakkis, 2010-03-20 13:26:34]

Thanks for taking the time to go through the whole patch Benjamin, I appreciate
it. Most issues are done, see my comments for the remaining ones.

Cheers,
George

http://codereview.appspot.com/659041/diff/1/4
File Doc/library/inspect.rst (right):

http://codereview.appspot.com/659041/diff/1/4#newcode507
Doc/library/inspect.rst:507: .. function:: getcallargs(func[, *args][, **kwds])
On 2010/03/20 03:14:31, Benjamin wrote:
> I would like to see it documented that
> 
> a) getcallargs() will raise exceptions similar to the ones you would get from
> invoking a function incorrectly.
> b) that it handles assigning self for bound methods.

Done.

http://codereview.appspot.com/659041/diff/1/4#newcode509
Doc/library/inspect.rst:509: Get the mapping of arguments to values when calling
``func(*args, **kwds)``.
On 2010/03/20 03:14:31, Benjamin wrote:
> How about "Bind the *args*, and *kwds* to the argument names of *func* as if
it
> was called with then."?

Done.

http://codereview.appspot.com/659041/diff/1/4#newcode511
Doc/library/inspect.rst:511: A dict is returned, with keys the function argument
names (including the
On 2010/03/20 03:14:31, Benjamin wrote:
> This can be in the same paragraph as the first phrase.

Done.

http://codereview.appspot.com/659041/diff/1/4#newcode515
Doc/library/inspect.rst:515: >>> from inspect import getcallargs
On 2010/03/20 03:14:31, Benjamin wrote:
> I don't think this line is needed.

You mean the import? It's used in other docs though, e.g. functools.rst.

http://codereview.appspot.com/659041/diff/1/2
File Lib/inspect.py (right):

http://codereview.appspot.com/659041/diff/1/2#newcode898
Lib/inspect.py:898: assigned_tuple_params = []
On 2010/03/20 03:14:31, Benjamin wrote:
> How about simply keeping all assigned names in a set? This will avoid a lot of
> complication.

getargspec() returns the tuple parameter names as a (possibly nested) list, so
they can't be added to a set or dict as is. Even if they were converted to
tuples though, we still have to iterate (recursively) though them to get to the
string params, so I don't see how it would simplify things.

http://codereview.appspot.com/659041/diff/1/2#newcode906
Lib/inspect.py:906: try: subvalue = next(value)
On 2010/03/20 03:14:31, Benjamin wrote:
> Please keep these on separate lines.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode918
Lib/inspect.py:918: #------- handle methods (bound and unbound)
--------------------------------
On 2010/03/20 03:14:31, Benjamin wrote:
> Remove all the "-" and all similar following comments.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode923
Lib/inspect.py:923: elif not positional or not isinstance(positional[0],
func.im_class):
On 2010/03/20 03:14:31, Benjamin wrote:
> Why are you typechecking this here? Why not just let the function call take
care
> of it?

Which function call ? We never call func() in getcallargs()

http://codereview.appspot.com/659041/diff/1/2#newcode932
Lib/inspect.py:932: num_defaults = len(defaults or ())
On 2010/03/20 03:14:31, Benjamin wrote:
> Conditional "len(default) if default else 0" is more expressive here.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode934
Lib/inspect.py:934: for arg,value in zip(args,positional):
On 2010/03/20 03:14:31, Benjamin wrote:
> Needs a space.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode937
Lib/inspect.py:937: if num_pos > num_args:
On 2010/03/20 03:14:31, Benjamin wrote:
> I don't think this conditional is needed.

It is, otherwise positional[-(num_pos-num_args):] would be a bogus non-empty
slice.

http://codereview.appspot.com/659041/diff/1/2#newcode946
Lib/inspect.py:946: elif num_args==0 and (num_pos or has_named):
On 2010/03/20 03:14:31, Benjamin wrote:
> Space between the == and operands.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode951
Lib/inspect.py:951: if isinstance(arg,str) and arg in named:
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in function call.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode954
Lib/inspect.py:954: "argument '%s'" % (f_name,arg))
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in tuple.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode958
Lib/inspect.py:958: for arg,value in zip(args[-num_defaults:],defaults):
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in zip() call.

Done.

http://codereview.appspot.com/659041/diff/1/2#newcode966
Lib/inspect.py:966: unexpected = unexpected.encode('ascii','replace')
On 2010/03/20 03:14:31, Benjamin wrote:
> You should actually encode it with sys.getdefaultencoding() and no error
> handler.

Changed 'ascii' to sys.getdefaultencoding() but the error handler should stay,
that's how regular function calls deal with it; there's already a test for it
that fails without the error handler.

http://codereview.appspot.com/659041/diff/1/2#newcode970
Lib/inspect.py:970: unassigned = sum(1 for arg in args if not is_assigned(arg))
On 2010/03/20 03:14:31, Benjamin wrote:
> Just use len on a listcomp.

I think "len(args) - len(filter(is_assigned, args))" looks even better.

http://codereview.appspot.com/659041/diff/1/3
File Lib/test/test_inspect.py (right):

http://codereview.appspot.com/659041/diff/1/3#newcode558
Lib/test/test_inspect.py:558: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Add a space before this line.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode564
Lib/test/test_inspect.py:564: 
On 2010/03/20 03:14:31, Benjamin wrote:
> You can split this eval() calls out into variables to avoid looking like Lisp.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode569
Lib/test/test_inspect.py:569: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Use self.assertRaises.

But then I can't get hold of the exception instance, can I ?

http://codereview.appspot.com/659041/diff/1/3#newcode587
Lib/test/test_inspect.py:587: 
On 2010/03/20 03:14:31, Benjamin wrote:
> How about execing a function definition instead of a lambda?

I used lambda mainly because it's 1 line instead of 3 for exec, plus you don't
have to pass or hardcode a function name. Any reason to exec a function instead?

http://codereview.appspot.com/659041/diff/1/3#newcode652
Lib/test/test_inspect.py:652: 
On 2010/03/20 03:14:31, Benjamin wrote:
> How about test_multiple features?

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode675
Lib/test/test_inspect.py:675: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Perhaps link to bug report?

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode681
Lib/test/test_inspect.py:681: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space between implicit tuple args.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode708
Lib/test/test_inspect.py:708: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space here.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode709
Lib/test/test_inspect.py:709: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Put "pass" on a new line.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode715
Lib/test/test_inspect.py:715: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in super() call.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode719
Lib/test/test_inspect.py:719: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space here, too.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode724
Lib/test/test_inspect.py:724: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in super call.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode728
Lib/test/test_inspect.py:728: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in super call.

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode732
Lib/test/test_inspect.py:732: 
On 2010/03/20 03:14:31, Benjamin wrote:
> self.assertFalse use case?

This is called only by other tests, so if the assertion fails it means some test
is broken, not the function being tested.

http://codereview.appspot.com/659041/diff/1/3#newcode734
Lib/test/test_inspect.py:734: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space between '+'

Done.

http://codereview.appspot.com/659041/diff/1/3#newcode738
Lib/test/test_inspect.py:738: 
On 2010/03/20 03:14:31, Benjamin wrote:
> Space in super call.

Done.

[Benjamin, 2010-03-20 16:32:15]

http://codereview.appspot.com/659041/diff/1/2
File Lib/inspect.py (right):

http://codereview.appspot.com/659041/diff/1/2#newcode923
Lib/inspect.py:923: elif not positional or not isinstance(positional[0],
func.im_class):
On 2010/03/20 13:26:34, George Sakkis wrote:
> Which function call ? We never call func() in getcallargs()

I mean why should this be typechecked here?

http://codereview.appspot.com/659041/diff/1/3
File Lib/test/test_inspect.py (right):

http://codereview.appspot.com/659041/diff/1/3#newcode569
Lib/test/test_inspect.py:569: 
On 2010/03/20 13:26:34, George Sakkis wrote:
> On 2010/03/20 03:14:31, Benjamin wrote:
> > Use self.assertRaises.
> 
> But then I can't get hold of the exception instance, can I ?

In 2.7, assertRaises returns a context manager with the exception on it.

[Benjamin, 2010-03-20 16:49:45]

I think it looks mostly good now.

http://codereview.appspot.com/659041/diff/13001/14001
File Doc/library/inspect.rst (right):

http://codereview.appspot.com/659041/diff/13001/14001#newcode511
Doc/library/inspect.rst:511: first argument (typically named ``self``) to the
bounding instance. A dict is
"bounding" -> "associated" I think

http://codereview.appspot.com/659041/diff/13001/14001#newcode512
Doc/library/inspect.rst:512: returned, with keys the function argument names
(including the names of the
How about ", mapping the argument names to their values."

http://codereview.appspot.com/659041/diff/13001/14002
File Lib/inspect.py (right):

http://codereview.appspot.com/659041/diff/13001/14002#newcode967
Lib/inspect.py:967: unassigned = len(args) - len(filter(is_assigned, args))
You can always use num_args instead of len(args) and replace filter with a list
comp.

[George Sakkis, 2010-03-20 20:48:27]

All done, we're good to go.

http://codereview.appspot.com/659041/diff/13001/14001
File Doc/library/inspect.rst (right):

http://codereview.appspot.com/659041/diff/13001/14001#newcode511
Doc/library/inspect.rst:511: first argument (typically named ``self``) to the
bounding instance. A dict is
On 2010/03/20 16:49:45, Benjamin wrote:
> "bounding" -> "associated" I think

Done.

http://codereview.appspot.com/659041/diff/13001/14001#newcode512
Doc/library/inspect.rst:512: returned, with keys the function argument names
(including the names of the
On 2010/03/20 16:49:45, Benjamin wrote:
> How about ", mapping the argument names to their values."

Done.

http://codereview.appspot.com/659041/diff/13001/14002
File Lib/inspect.py (right):

http://codereview.appspot.com/659041/diff/13001/14002#newcode967
Lib/inspect.py:967: unassigned = len(args) - len(filter(is_assigned, args))
On 2010/03/20 16:49:45, Benjamin wrote:
> You can always use num_args instead of len(args) and replace filter with a
list
> comp.

Done.

[George Sakkis, 2010-03-21 21:51:54]

Updated to align with the changes of r79235 and r79237