Do not treat arguments that start with '--' as string. #99
Conversation
dbieber
left a comment
There was a problem hiding this comment.
This is great, I wish I'd noticed your pull request sooner. 2 comments inline.
fire/core.py
Outdated
| arg_consumed = True | ||
| else: | ||
| unconsumed_named_args.append(argument) | ||
| arg_consumed = True |
There was a problem hiding this comment.
We can remove arg_consumed altogether (here and at 698), and replace the if at 739 with an else.
| 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. |
There was a problem hiding this comment.
- Please update the Returns: section of the docstring.
- Also how about "remaining_kwargs" instead of unconsumed_named_args for consistency with the existing terminology?
|
Thanks! I'll look into it. |
|
Heads up that some of the tests are failing. You can see the details here (https://travis-ci.org/google/python-fire/jobs/287352133) or by clicking "Details" next to the continuous-integration line after "Some checks were not successfull". It looks like the majority of the failures are using the components tc.Kwargs or tc.MixedDefaults. A good first place to investigate might be why this assert is failing [1] [2]: Lines 344 to 346 in b5053ba Here's the definition of MixedDefaults: python-fire/fire/test_components.py Lines 74 to 83 in a0791c9 For what it's worth, these components are also used in many of the failing tests: python-fire/fire/test_components.py Lines 56 to 62 in a0791c9 python-fire/fire/test_components.py Lines 175 to 184 in a0791c9 |
| remaining_kwargs = [] | ||
| remaining_args = [] | ||
|
|
||
| if not args: |
There was a problem hiding this comment.
I add a return statement here because pylint complains about "Too many nested blocks (6/5)". Not sure if I should do it this way.
There was a problem hiding this comment.
Seems ok to me.
Another possibility would be moving some of the inner blocks into their own functions, but I don't see a super clean way to do that.
|
@dbieber Thanks for the tip! I forgot to save the value corresponding to the keyword. |
| """Parses the list of `args` into (varargs, kwargs), remaining_args.""" | ||
| kwargs, remaining_args = _ParseKeywordArgs(args, all_args, fn_spec.varkw) | ||
| kwargs, remaining_kwargs, remaining_args = \ | ||
| _ParseKeywordArgs(args, all_args, fn_spec.varkw) |
There was a problem hiding this comment.
nit: for consistency w/ codebase, let's do:
kwargs, remaining_kwargs, remaining_args = _ParseKeywordArgs(
args, all_args, fn_spec.varkw)
| remaining_kwargs = [] | ||
| remaining_args = [] | ||
|
|
||
| if not args: |
There was a problem hiding this comment.
Seems ok to me.
Another possibility would be moving some of the inner blocks into their own functions, but I don't see a super clean way to do that.
| fire.Fire( | ||
| tc.MixedDefaults, | ||
| command=['identity', '--alpha', 'True', '"--test"']), | ||
| (True, '--test')) |
| self.assertEqual( | ||
| fire.Fire(tc.MixedDefaults, | ||
| command=['identity', 'True', '10']), (True, 10)) | ||
|
|
There was a problem hiding this comment.
Another thing that may be worthwhile to test is the behavior in the presence of separators / chained function calls.
For example, with these components:
def example_component(arg1, kwarg2='default2'):
def inner_component(kwarg3='default3'):
return kwarg3
return inner_component
def example_component(arg1, *varargs):
def inner_component(kwarg3='default3'):
return kwarg3
return inner_component
What is the behavior of these calls?
fire.Fire(example_component, command=['value1', '-', '--kwarg3', 'value3']) == 'value3'
fire.Fire(example_component, command=['value1', '--kwarg3', 'value3'])
|
I'm going to go ahead and squash + merge. If you're up for adding those tests and/or making the nit fix, go ahead and do so in a follow up PR. |
Fixes #84