Fix keyword-only arguments #18

jgarst · 2017-03-11T20:59:53Z

Should fix the rest of #6.

Also modify tests for inspectutils.GetArgSpec.

Keyword only arguments are now allowed, and are required to be passed with flags.

dbieber · 2017-03-11T22:20:20Z

Awesome! I'll look through the pull request later in the week.

We'll also need to get travis passing before we can check this in, and there'll be some style things to fix up too. My first thought for how to make Travis happy is to conditionally exclude files from nosetest (called in .travis.yml atm) based on the python version, which I think Travis exposes as an env variable. Open to suggestions.

jgarst · 2017-03-12T02:18:03Z

Ach, I missed that you included a travis file, and tried to test by hand. Sorry about that.

That nose test discovery is annoyingly broad. I suppose I like starting a nose exclude list, if the alternative is removing the word 'test' from test related non-scripts. I've updated the nose command in travis.yml. An alternative might be to create a separate test directory. Then testing components would not need 'test' in their name for clarity, and so would not be discovered and run automatically. This is getting way into the minutia though.

Let me know about the style issues. I tried to be conservative and match yours, but I will have of course missed some bits.

The non-CI related problem travis discovered is a bug in python < 3.4.3 namedtuple._asdict. Specifically subclasses of namedtuple always returning an empty dictionary.

The workaround isn't that ugly, but having a datatype with that much of a gotcha is a landmine for development. Unless you are looking for an excuse to drop support for python 3.4.0 - 3.4.3, I'll figure out some way to avoid it.

dbieber

Thanks for doing this.

Your update to .travis.yml sounds good for now. If/when we end up needing to grow the exclude list further, we can reevaluate.
Thoughts inline about using a class rather than a namedtuple.
I also put comments on each of the style bits. Gotta keep the linter happy :). (We should set up Travis to check lint automatically...)

dbieber · 2017-03-12T04:33:25Z

fire/core.py

@@ -546,16 +546,19 @@ def _MakeParseFn(fn):
    can then be called with fn(*varargs, **kwargs). The remaining_args are
    the leftover args from the arguments to the parse function.
  """
-  fn_args, fn_varargs, fn_keywords, fn_defaults = inspectutils.GetArgSpec(fn)
+  (fn_args, fn_varargs, fn_keywords, fn_defaults, fn_kwonly,
+    fn_kwonlydefaults, fn_annotations) = inspectutils.GetArgSpec(fn)


style: 4 space indent after paren

dbieber · 2017-03-12T04:38:16Z

fire/core.py

+
+    missing_kwonly = set(required_kwonly) - set(kwargs)
+    if missing_kwonly:
+      raise FireError('Missing required flags:', missing_kwonly)


How about something like 'Missing required keyword-only args'

Do you plan to keep this error printing to the terminal when a user mistypes something on the command line? Someone using the final command line program might be familiar with command line flags, but not the python specific concept of keyword only arguments.

That's a fair point.
Yeah, currently this error does surface to the user, so your string sgtm.
There's definitely room to clean up the Fire trace output, so we can think about changing how these errors are displayed if/when we go down that route.

dbieber · 2017-03-12T04:38:27Z

fire/core.py

@@ -580,7 +588,7 @@ def _ParseFn(args):

    varargs = parsed_args + varargs

-    consumed_args = args[:len(args)-len(remaining_args)]
+    consumed_args = args[:len(args) - len(remaining_args)]


dbieber · 2017-03-12T04:47:17Z

fire/fire_test.py

@@ -102,6 +103,14 @@ def testFireAnnotatedArgs(self):
    self.assertEqual(fire.Fire(tc.Annotations, 'double 5'), 10)
    self.assertEqual(fire.Fire(tc.Annotations, 'triple 5'), 15)

+  @unittest.skipIf(sys.version_info < (3, 0),


Do we want this or six.PY2?
We've been using six.PY2, so I lean toward six for consistency.

You are correct, and I have fixed it, but these testing libraries tend to only get more version specific over time. You might end up getting back to sys.version_info anyway.

dbieber · 2017-03-12T04:47:56Z

fire/test_components_py3.py

+  return arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10
+
+
+class KeywordOnly:


class KeywordOnly(object):

Fixed, but since this file can't be parsed by python 2 anyway, does it matter?

dbieber · 2017-03-12T05:14:25Z

fire/inspectutils.py

+from collections import namedtuple
+
+_FullArgTuple = namedtuple('FullArgSpec', ('args', 'varargs', 'varkw',
+  'defaults', 'kwonlyargs', 'kwonlydefaults', 'annotations'))


format this as:

_FullArgTuple = namedtuple( 'FullArgSpec', ('args', 'varargs', 'varkw', 'defaults', 'kwonlyargs', 'kwonlydefaults', 'annotations'))

dbieber · 2017-03-12T05:14:53Z

fire/inspectutils.py

+  """NamedTuple storing arguments of a callable, populated with defaults."""
+
+  def __new__(cls, args=None, varargs=None, varkw=None, defaults=None,
+    kwonlyargs=None, kwonlydefaults=None, annotations=None):


align kwonlyargs with cls.

dbieber · 2017-03-12T05:26:59Z

fire/inspectutils.py

+  'defaults', 'kwonlyargs', 'kwonlydefaults', 'annotations'))
+
+
+class _FullArgSpec(_FullArgTuple):


Thinking aloud:
Is a class that inherits from a namedtuple really the best choice here?
My initial aversion is that it feels clunky. Also, now that we have 7 args, it strikes me that variable expansion is prone to breaking if we mess up the argument order somewhere and don't notice.
My preference would be to abandon the tuple aspect. And instead I think we should just have a class, and always access the members by name. Something like:

class FullArgSpec(object): """The full argspec of a callable.""" def __init__(self, args=None, varargs=None, varkw=None, defaults=None, kwonlyargs=None, kwonlydefaults=None, annotations=None): self.args = args or [] self.varargs = varargs self.varkw = varkw self.defaults = defaults or () self.kwonlyargs = kwonlyargs or [] self.kwonlydefaults = kwonlydefaults or {} self.annotations = annotations or {}

How's that sound?

I agree, the tuple is a holdover from python3's inspect.getargspec that I did not reevaluate.

As this object adds more information, it might be wise to converge it to python 3's inspect.Signature object, which seems to be the canonical future of signatures in python.

Signature has an ordered dict of Parameter (name, kind, default, annotation) objects. I think this might make cleaner parsing in core._ParseFn, and I worry that without this representation your signature and python3's signature will evolve in different directions. It is also probably overpowered for what you need, and an example of small pull requests turning into larger pull requests.

The right answer probably depends on your ideas about the scope of this project. I'm already mucking about here, and happy to modify it as you like.

dbieber · 2017-03-12T05:30:27Z

fire/helputils.py

-    return _UsageStringFromFnDetails(command, args, varargs, keywords, defaults)
+    spec = inspectutils.GetArgSpec(component)
+    spec_dict = dict(zip(spec._fields, iter(spec)))
+    return _UsageStringFromFnDetails(command, **spec_dict)


in line w/ the previous comment, this would become _UsageStringFromFnDetails(command, spec).

dbieber · 2017-03-12T05:37:44Z

fire/test_components.py

@@ -17,10 +17,20 @@
 from __future__ import absolute_import
 from __future__ import division
 from __future__ import print_function
+import six


new line before import

dbieber · 2017-03-12T05:51:41Z

Also make sure you read this comment first, because a number of the style comments become irrelevant after switching from namedtuple to class.

jgarst · 2017-03-14T00:06:33Z

Thanks for taking the time to review this, by the way.

Quick question, after making the spec class a simple object, the test testGetArgSpecReturnType becomes.

` def testGetArgSpecReturnType(self):
# Asserts that the object returned by GetArgSpec has the appropriate fields.
spec = inspectutils.GetArgSpec(tc.identity)

self.assertTrue(hasattr(spec, 'args'))
self.assertTrue(hasattr(spec, 'defaults'))
self.assertTrue(hasattr(spec, 'varargs'))
self.assertTrue(hasattr(spec, 'varkw'))
self.assertTrue(hasattr(spec, 'kwonlyargs'))
self.assertTrue(hasattr(spec, 'kwonlydefaults'))
self.assertTrue(hasattr(spec, 'annotations'))`

It is not totally clear what this is testing anymore, that other tests aren't doing implicitly. My inclination would be to remove it, but I didn't want to do that without comment. Let me know what you think.

dbieber · 2017-03-14T00:19:27Z

Yes, I think it's good to remove that testGetArgSpecReturnType test now.

dbieber

Looks good! Some small things inline.

dbieber · 2017-03-14T02:55:37Z

fire/helputils.py

@@ -136,16 +136,19 @@ def _UsageStringFromFnDetails(command, args, varargs, keywords, defaults):
    command: The command leading up to the function.


The docstring should now just list command and spec as args.

dbieber · 2017-03-14T03:01:20Z

fire/inspectutils.py

@@ -63,46 +77,40 @@ def GetArgSpec(fn):
  Args:
    fn: The function to analyze.
  Returns:
-    A named tuple of type inspect.ArgSpec with the following fields:
+    An object containing the following fields:
      args: A list of the argument names accepted by the function.


Let's move these arg descriptions to the docstring of _FullArgSpec's init, and then the docstring here can be much simpler, something like:

... Returns: A FullArgSpec object for the provided function.

Oops. Missed this pretty bad.

dbieber · 2017-03-14T03:02:26Z

fire/inspectutils.py

@@ -24,6 +24,20 @@
 import six


+class _FullArgSpec(object):


This is used outside of this module, albeit never reference by name outside of this module. So, I think we should drop the underscore. What do you think?

I think you are right, my hesitation is that once it is a public non-builtin type it should be able to instantiate itself from a function, and other things people will reasonably expect of it.

I suppose this isn't really a reason not to do it, but it will be trickier to decide what it's job is once it is publically visible.

dbieber · 2017-03-14T03:11:26Z

fire/inspectutils.py

-      keywords = argspec.keywords
+      args = argspec.args[1:] if skip_arg else argspec.args
+      annotations = getattr(fn, '__annotations__', None)
+      fullspec = _FullArgSpec(args, *argspec[1:], annotations=annotations)


This makes me a little nervous.

Brainstorming why:
1: It hides which arguments are being passed from getargspec to _FullArgSpec,
2: subject to breaking if either getargspec or _FullArgSpec change (not really a concern I guess)
3: Reduces the approachability of the codebase (as a side effect of 1, plus using slices and *args in the same fn call).

None of these are big issues and this is nicely compact, so I guess it's fine to leave. Up to you.

Applies to line 101 as well.

It is probably too clever. I'll stop inling the if and just pop off the first argument. I'll probably change FullArgSpec so that it takes a function. Then passing hidden arguments won't be a problem. Since FullArgSpec no longer really behaves like inspect.FullArgSpec, it probably also needs a new name.
``

dbieber · 2017-03-14T03:15:38Z

fire/inspectutils.py


  except TypeError:
-    args = []
-    defaults = ()
    # If we can't get the argspec, how do we know if the fn should take args?
    # 1. If it's a builtin, it can take args.
    # 2. If it's an implicit __init__ function (a 'slot wrapper'), take no args.
    # Are there other cases?


[Unrelated comment]
Heads up that yes, there are other cases, so we'll have to change this logic a little bit (not as part of this review, of course). This does not affect your change and will go in after.
The cases we originally missed are C-extensions, which inspect.signature/inspect.fullargspec cannot get the argspec for. While we can't solve this in the general case, we can add known argspecs for builtin modules. The only ones on my list to add so far are object.__init__ (which gets used a lot and which we already handle correctly) and type.__init__ (which is much less used, but still important).

Yeah, callable objects too, like functools.partial. There is a 50/50 chance I am missing something, but I think those are basically all the cases.

You will probably be special casing forever of course. The first one should probably be special logic to grab the real arguments from the partial object, instead of the generic *args, **kwargs it provides. My assumption is that you will eventually want something that is or looks like single dispatch here.

dbieber · 2017-03-14T03:18:31Z

fire/inspectutils_test.py

+    self.assertEqual(spec.annotations, {'arg2': int, 'arg4': int})
+
+
+  @unittest.skipIf(six.PY2, "No keyword arguments in python 2")


single quotes

dbieber · 2017-03-14T03:18:46Z

fire/fire_test.py

@@ -102,6 +102,13 @@ def testFireAnnotatedArgs(self):
    self.assertEqual(fire.Fire(tc.Annotations, 'double 5'), 10)
    self.assertEqual(fire.Fire(tc.Annotations, 'triple 5'), 15)

+  @unittest.skipIf(six.PY2, "keyword only arguments not supported in python 2")


single quotes

dbieber · 2017-03-14T22:01:21Z

I hope you don't mind I pushed some changes directly to the pull request rather than leaving comments. See them here 157200e.

dbieber · 2017-03-14T22:06:46Z

I'll wait for you to look through those changes before I move anything into google/python-fire:master. Let me know if you have comments.

Re: the naming changes:
While it probably would have been better to go with lower case function names throughout the project, I don't want to partially switch over. I want to keep consistency within the project.

jgarst · 2017-03-15T01:24:34Z

Haha, no problem. I'm just sorry the long tail of this has taken so much of your time.

In terms of the rename, I wasn't trying to change the naming convention. I was more worried about the conceptual break that has happened with inspect.FullArgSpec, and then the problem of having the constructor for a Specification from a function is called GetArgSpecs. Someone who knew they needed a Specification object might not immediately think to call that.

Maybe this worry is overblown though, and you are the only one with a good sense for the cost of refactoring things.

Jared Garst added 3 commits March 11, 2017 11:14

Add tests for keyword only arguments.

9a6df3c

Also modify tests for inspectutils.GetArgSpec.

Fix keyword only arguments.

d449c50

Keyword only arguments are now allowed, and are required to be passed with flags.

Print keyword only arguments in help.

784f112

dbieber changed the title ~~Fix keyword arguments~~ Fix keyword-only arguments Mar 11, 2017

Fix travis errors.

fc78fd0

dbieber reviewed Mar 12, 2017

View reviewed changes

dbieber reviewed Mar 14, 2017

View reviewed changes

Jared Garst and others added 2 commits March 14, 2017 09:06

Changed spec from namedtuple to object, style fixes.

4a6ade8

Naming and formatting changes, primarily around GetFullArgSpec.

157200e

dbieber merged commit babf4f1 into google:master Mar 20, 2017

dbieber mentioned this pull request Mar 20, 2017

annotations and kwonlyargs #6

Closed

		return arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9, arg10


		class KeywordOnly:

		'defaults', 'kwonlyargs', 'kwonlydefaults', 'annotations'))


		class _FullArgSpec(_FullArgTuple):

		@@ -136,16 +136,19 @@ def _UsageStringFromFnDetails(command, args, varargs, keywords, defaults):
		command: The command leading up to the function.

		self.assertEqual(spec.annotations, {'arg2': int, 'arg4': int})


		@unittest.skipIf(six.PY2, "No keyword arguments in python 2")

Fix keyword-only arguments #18

Fix keyword-only arguments #18

Conversation

jgarst commented Mar 11, 2017

dbieber commented Mar 11, 2017

jgarst commented Mar 12, 2017

dbieber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbieber commented Mar 12, 2017

jgarst commented Mar 14, 2017

dbieber commented Mar 14, 2017

dbieber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbieber commented Mar 14, 2017

dbieber commented Mar 14, 2017

jgarst commented Mar 15, 2017