Better handling of argument name typos

[gerardlouw]

Using this minimal example of a Python-fire CLI:

import fire

def f(my_arg):
    print(my_arg)

fire.Fire(my_arg)

This is the result I get:

➜  ~ python example.py --my-arg=hello
hello
➜  ~ python example.py --myarg=hello
--myarg=hello

As you can see, a typo in the argument leads to unexpected behaviour, which can be really hard to debug.

I would suggest never treating an argument matching --[a-zA-Z0-9_-]+ as positional, without some sort of escaping by the user (perhaps similar to the escaping you require to avoid parsing ints, etc.).

[dbieber]

Thanks for this suggestion! It's a good idea, and I will let you know if we decide to go with this design change in a future version.

[dbieber]

@gerardlouw, would you be interested in making this improvement?

[gerardlouw]

Sure, I'd be happy to send a pull request. Could you point me to where in the code this should go? From a quick glance, it looks like

python-fire/fire/core.py

Lines 663 to 741 in e76a10c

	def _ParseKeywordArgs(args, fn_args, fn_keywords):
	"""Parses the supplied arguments for keyword arguments.
	Given a list of arguments, finds occurences of --name value, and uses 'name'
	as the keyword and 'value' as the value. Constructs and returns a dictionary
	of these keyword arguments, and returns a list of the remaining arguments.
	Only if fn_keywords is None, this only finds argument names used by the
	function, specified through fn_args.
	This returns the values of the args as strings. They are later processed by
	_ParseArgs, which converts them to the appropriate type.
	Args:
	args: A list of arguments
	fn_args: A list of argument names that the target function accepts,
	including positional and named arguments, but not the varargs or kwargs
	names.
	fn_keywords: The argument name for **kwargs, or None if **kwargs not used
	Returns:
	kwargs: A dictionary mapping keywords to values.
	remaining_args: A list of the unused arguments from the original args.
	"""
	kwargs = {}
	if args:
	remaining_args = []
	skip_argument = False
	for index, argument in enumerate(args):
	if skip_argument:
	skip_argument = False
	continue
	arg_consumed = False
	if argument.startswith('--'):
	# This is a named argument; get its value from this arg or the next.
	got_argument = False
	keyword = argument[2:]
	contains_equals = '=' in keyword
	is_bool_syntax = (
	not contains_equals and
	(index + 1 == len(args) or args[index + 1].startswith('--')))
	if contains_equals:
	keyword, value = keyword.split('=', 1)
	got_argument = True
	elif is_bool_syntax:
	# Since there's no next arg or the next arg is a Flag, we consider
	# this flag to be a boolean.
	got_argument = True
	if keyword in fn_args:
	value = 'True'
	elif keyword.startswith('no'):
	keyword = keyword[2:]
	value = 'False'
	else:
	value = 'True'
	else:
	if index + 1 < len(args):
	value = args[index + 1]
	got_argument = True
	keyword = keyword.replace('-', '_')
	# In order for us to consume the argument as a keyword arg, we either:
	# Need to be explicitly expecting the keyword, or we need to be
	# accepting **kwargs.
	if got_argument and (keyword in fn_args or fn_keywords):
	kwargs[keyword] = value
	skip_argument = not contains_equals and not is_bool_syntax
	arg_consumed = True
	if not arg_consumed:
	# The argument was not consumed, so it is still a remaining argument.
	remaining_args.append(argument)
	else:
	remaining_args = args
	return kwargs, remaining_args

might be the right place.

[dbieber]

The call paths we're interested in are: (_Fire -> _CallCallable -> _MakeParseFn) and (_Fire -> _CallCallable -> parse -> _ParseArgs).

The parse function is defined here (_ParseFn):

python-fire/fire/core.py

Lines 566 to 567 in e76a10c

	def _ParseFn(args):
	"""Parses the list of `args` into (varargs, kwargs), remaining_args."""

In particular we're interested in the else block here:

python-fire/fire/core.py

Lines 638 to 644 in e76a10c

	if value is not None: # A value is specified at the command line.
	value = _ParseValue(value, index, arg, metadata)
	parsed_args.append(value)
	else: # No value has been explicitly specified.
	if remaining_args and accepts_positional_args:
	# Use a positional arg.
	value = remaining_args.pop(0)

Line 644 is where we indiscriminately take one of the remaining args and treat it as a positional arg.
Instead, we should take the first arg that doesn't start with -- and use that.

[greenmoon55]

Just for clarification, will we consider these variables for varargs?

[dbieber]

What do you think about the following?

If:

• an argument starts with '--' (e.g. --arg)
• AND the argument is not surrounded in quotes (in practice this means being double surrounded in quotes :/) e.g. ('"--arg"')
• AND the argument is not prefixed with an equal sign (e.g. --someflag=--arg)

Then:

• We shouldn't treat the arg as a string at all, since the user probably meant for the arg to be a flag, not a value.

[greenmoon55]

Thanks! Can I remove these arguments in '_ParseKeywordArgs' to avoid being assigned to varargs?

[dbieber]

Ack! I only just noticed your question now. Sorry for the delay.

Yes, handling these arguments in _ParseKeywordArgs sounds reasonable to me.
You could modify _ParseKeywordArgs to return kwargs, remaining_args, unused_kwargs instead of kwargs, remaining_args, where unused_kwargs is a list of any keyword args that weren't used.
By separating these unused keyword args from remaining_args, we prevent these args that were intended to be keyword args from accidentally being used as positional args.
Then, at the end of _ParseFn, we can add the unused_kwargs back into remaining_args, in case they're used later on in _Fire.

Edit: Oh, and I see you've already done something very similar to this. I'll take a look at it now.

[dbieber]

Thanks for the contribution!

[greenmoon55]

Thanks for your help!