Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[parser] Support nargs=+ in parse_kwargs #3930

Merged
merged 1 commit into from
Aug 13, 2021
Merged

Conversation

stephenroller
Copy link
Contributor

@stephenroller stephenroller commented Aug 11, 2021

Patch description
@ankitade discovered that parse_kwargs doesn't work with choices and nargs='+'. This bug has actually been around a surprising amount of time. Patch and test.

Testing steps
New CI

@stephenroller stephenroller marked this pull request as ready for review August 11, 2021 19:24
Copy link
Contributor

@moyapchen moyapchen left a comment

Choose a reason for hiding this comment

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

Lgtm

@@ -1223,7 +1223,7 @@ def _kwargs_to_str_args(self, **kwargs):
string_args.append(self._value2argstr(value))
elif isinstance(action, argparse._StoreAction) and action.nargs in '*+':
string_args.append(last_option_string)
string_args.extend([self._value2argstr(value) for v in value])
string_args.extend([self._value2argstr(v) for v in value])
Copy link
Contributor

Choose a reason for hiding this comment

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

Oof

@@ -190,6 +190,25 @@ def test_parse_kwargs_multirounds(self):
with self.assertRaises(KeyError):
parser.parse_kwargs(task='integration_tests', fake_option=False)

def test_parse_kwargs_nargsplus(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Ty for tests

@stephenroller stephenroller merged commit cdc8c05 into master Aug 13, 2021
@stephenroller stephenroller deleted the parse_manyargs2 branch August 13, 2021 20:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants