Skip to content
44 changes: 38 additions & 6 deletions fire/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def main(argv):
import shlex
import sys
import types
import re

from fire import completion
from fire import decorators
Expand Down Expand Up @@ -750,7 +751,7 @@ def _ParseArgs(fn_args, fn_defaults, num_required_args, kwargs,
def _ParseKeywordArgs(args, fn_spec):
"""Parses the supplied arguments for keyword arguments.

Given a list of arguments, finds occurences of --name value, and uses 'name'
Given a list of arguments, finds occurrences 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.

Expand All @@ -767,6 +768,9 @@ def _ParseKeywordArgs(args, fn_spec):
kwargs: A dictionary mapping keywords to values.
remaining_kwargs: A list of the unused kwargs from the original args.
remaining_args: A list of the unused arguments from the original args.
Raises:
FireError: if a boolean shortcut arg is passed that could refer to multiple
args
"""
kwargs = {}
remaining_kwargs = []
Expand All @@ -785,15 +789,27 @@ def _ParseKeywordArgs(args, fn_spec):
continue

arg_consumed = False
if argument.startswith('--'):
if _IsFlag(argument):
# This is a named argument; get its value from this arg or the next.
got_argument = False

keyword = argument[2:]
keyword = ''
if _IsSingleCharFlag(argument):
keychar = argument[1]
potential_args = [arg for arg in fn_args if arg[0] == keychar]
if len(potential_args) == 1:
keyword = potential_args[0]
elif len(potential_args) > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need to handle the len(potential_args) == 0 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also throw an error in this case? My original assumption here was that if the flag doesn't match, we would assume it was an value for another flag. Now that I think about it though this could cause confusing behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we should throw an error in this case.
If the argument looks like a flag (_IsFlag), it shouldn't be acceptable as a value.
[And I think we'll also broaden what _IsFlag accepts too in a separate change.]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that sounds good to me, I'll add a branch and a test case for it

raise FireError("The argument '{}' is ambiguous as it could "
"refer to any of the following arguments: {}".format(
argument, potential_args))

else:
keyword = argument[2:]

contains_equals = '=' in keyword
is_bool_syntax = (
not contains_equals and
(index + 1 == len(args) or args[index + 1].startswith('--')))
is_bool_syntax = (not contains_equals and
(index + 1 == len(args) or _IsFlag(args[index + 1])))
if contains_equals:
keyword, value = keyword.split('=', 1)
got_argument = True
Expand Down Expand Up @@ -828,13 +844,29 @@ def _ParseKeywordArgs(args, fn_spec):
if skip_argument:
remaining_kwargs.append(args[index + 1])


if not arg_consumed:
# The argument was not consumed, so it is still a remaining argument.
remaining_args.append(argument)

return kwargs, remaining_kwargs, remaining_args


def _IsFlag(argument):
"""Determines if the argument is a flag argument"""
return _IsSingleCharFlag(argument) or _IsMultiCharFlag(argument)


def _IsSingleCharFlag(argument):
"""Determines if the argument is a single char flag (e.g. '-a')"""
return re.match('^-[a-z]$', argument)


def _IsMultiCharFlag(argument):
"""Determines if the argument is a multi char flag (e.g. '--alpha')"""
return argument.startswith('--')


def _ParseValue(value, index, arg, metadata):
"""Parses value, a string, into the appropriate type.

Expand Down
25 changes: 25 additions & 0 deletions fire/fire_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,31 @@ def testBoolParsingLessExpectedCases(self):
fire.Fire(tc.MixedDefaults, command=r'identity --alpha \"--test\"'),
('--test', '0'))

def testBoolShortcutParsing(self):
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '-a']), (True, '0'))
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '-a', '--beta=10']), (True, 10))
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '-a', '-b']), (True, True))
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '-a', '42', '-b']), (42, True))
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '-a', '42', '-b', '10']), (42, 10))
self.assertEqual(
fire.Fire(tc.MixedDefaults,
command=['identity', '--alpha', 'True', '-b', '10']),
(True, 10))
with self.assertRaisesFireExit(2):
# This test attempts to use a boolean shortcut on a function with
# a naming conflict for the shortcut, triggering a FireError
fire.Fire(tc.SimilarArgNames, command=['identity', '-b'])

def testBoolParsingWithNo(self):
# In these examples --nothing always refers to the nothing argument:
def fn1(thing, nothing):
Expand Down
6 changes: 6 additions & 0 deletions fire/test_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ def identity(self, alpha, beta='0'):
return alpha, beta


class SimilarArgNames(object):

def identity(self, bool_one=False, bool_two=False):
return bool_one, bool_two


class Annotations(object):

def double(self, count=0):
Expand Down