Accept argv list in calls to Fire

[dbieber]

Currently Fire() uses sys.argv. It should also be able to accept an argv passed in directly to the call to Fire.

Some considerations:

• Currently Fire() accepts an undocumented argument command which is a string. Do we still want to accept a string (if yes, do we continue using the name command or do we just use the name argv?
• If we remove the command argument, we can use BigQuery to find all instances of people using this argument and send them a pull request fixing it. It's undocumented, so 1) I don't think we have any obligation to do this, and 2) there shouldn't be many people using this argument, so sending the pull requests shouldn't be too big a job.
• Does the passed in argv need argv[0] to be the program name?

[jtratner]

just a thought - I think it'd be more consistent to accept the equivalent of argv[1:]

e.g. here's snippet from argparse.py:

        if args is None:
            args = _sys.argv[1:]

https://svn.python.org/projects/python/trunk/Lib/argparse.py

[dbieber]

Yeah, that may make more sense.

One option that preserves backwards compatibility is

def Fire(component, command=None, name=None):
  name = name or os.path.basename(sys.argv[0])  # (Note: Currently we don't set name from sys.argv[0] if command is set, and I think that's a bug.)
  if isinstance(command, six.string_types):
    args = shlex.split(command)
  elif isinstance(command, (list, tuple)):
    args = command
  elif command is None:
    args = sys.argv[1:]
  else:
    raise ValueError()

I would rather "get it right" than preserve backwards compatibility since the project is young and these are undocumented and largely unused arguments.

I'm tempted to (once completion is updated) remove the name argument altogether and always use the default of os.path.basename(sys.argv[0]). What do you think of this? (e.g. is it a problem people are using aliases? what are the use cases where the CLI name should differ from sys.argv[0]? How should name be set when command/args is supplied?)

We should also try to get the argument names right:
argparse uses the argument prog whereas we currently use name.
For the arguments/command, we could do command, args, argv. To me, command sounds like a string, args sounds like the list sys.argv[1:], and argv sounds like the list sys.argv.

I think my preference is:

def Fire(component, args=None, name=None):
  name = name or os.path.basename(sys.argv[0])
  if isinstance(args, six.string_types):
    args = shlex.split(args)
  elif args is None:
    args = sys.argv[1:]
  if not isinstance(args, (list, tuple)):
    raise ValueError()

I think it's ok that this renames command and therefore isn't backwards compatible w/ that undocumented argument. I also think it's OK that args can accept a string as well as a list. Using the name args over command encourages people to pass a list, which is the better/safer way to use this, though passing a string is the 'easier' way.

One concern here is that if we later remove the name argument, that leaves no way to change the name used in the usage strings short of modifying sys.argv (which I definitely don't want to encourage). e.g. you can't pass in an args list w/ args[0] set differently. Maybe I'm wrong to want to remove the name argument :).

[jtratner]

I agree that breaking backwards compatibility is the right choice here - especially if you can carry over knowledge from argparse to fire.

My bikeshed:

• agree on args over command.
• name vs. prog - eh - click doesn't even allow you to set name, but name is possibly more straightforward than prog. /shrug

I believe the reason you'd want to overwrite name is when aliasing commands (like gvim / vim / vimdiff / etc). But agreed I'm not sure why you'd overwrite it.

[nealmcb]

I think I agree with both of you there. Make a clean interface, via args rather than command. I'm not sure how strong the use case is for name, but there may well be one around usage strings as you note. But I recognize there are lots of subtleties here, and I'm not up-to-speed with all of the issues.

[dbieber]

For some context, right now we use name in two places:

1. Usage strings
2. Completion script generation

And there are two times I can think of where you'd maybe (?) want to set name explicitly:

1. If the command is commonly called by an alias
2. If the command is commonly called inside of another script w/ a different name (if you want usage/completion to work with that outer script).

It's worth noting here that it's possible for a completion script to do alias resolution, though we don't do it yet.

I also need to think more about what name + usage strings should look like if you call Fire from inside a function that was already called by Fire (as we will likely do in #29).

[dbieber]

Having thought more about this, I'm inclined to keep the "command" argument rather than replacing it with "args". We'll make it accept both a list of args or a string. It does after all represent the Fire command, and while args would also be a suitable (perhaps better!) name, I've come to think that the backwards compatibility argument outweighs this. [I don't want to come under fire for breaking the API!]

[Relatedly, there is also precedent for accepting a command both as a string or a list of arguments (subprocess.py), though they use the name args.]

I also need to think more about what name + usage strings should look like if you call Fire from inside a function that was already called by Fire (as we will likely do in #29).

If you call Fire with:

• no command and no name -> command=sys.argv[1:], name=sys.argv[0]
• no command and a name -> command=sys.argv[1:], name=name
• command and no name -> command=command, name=sys.argv[0] or None(?)
• command and name -> command=command, name=name

The third case (command is supplied but name is not) is the most interesting one. I'm inclined to use sys.argv[0] in this case, but am also considering removing the name from the usage strings altogether in this case.

When calling Fire from a method launched with Fire, one will either:
a) want access to the original unparsed arguments,
b) want Fire to not further parse the arguments.

We unofficially enable a) with the decorator in decorators.py @SetParseFn(str), but we'll want to make it official for this to be useful. We don't support b.

[dbieber]

Resolved with b5053ba.