bpo-29626: spacing issue when using help in argparse

[csabella]

A metavar value of an empty string was causing an extra space to be printed in argparse help.

For example, the created test case was printing:
-f , --foo (extra space before the comma)
instead of
-f, --foo

[mention-bot]

@csabella, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @bethard and @bitdancer to be potential reviewers.

[bitdancer]

Please add comments to the issue about your proposed solution.

[serhiy-storchaka]

(option_string) is not a 1-element tuple.

[csabella]

I've fixed it. I haven't worked with the older version of string formatting, so I wasn't aware I did a hybrid of the two methods. It seems that a 1-element tuple is preferred, so I've changed it to be that.

[csabella]

@bitdancer Thank you. I've added a comment to the issue tracker.

[serhiy-storchaka]

If option_string is a string, '%s' % (option_string,) can be replaced with just option_string. And the loop can be replaced with the single extend() call as at line 548.

But I'm not convinced that supporting empty names is a good idea.

[csabella]

Thanks @serhiy-storchaka . I've been looking at this trying to figure out what you meant by using a single extend() call.

The original code is:

else:
    default = self._get_default_metavar_for_optional(action)
    args_string = self._format_args(action, default)
    for option_string in action.option_strings:
        parts.append('%s %s' % (option_string, args_string))

So, are you saying that this change would be:

else:
    default = self._get_default_metavar_for_optional(action)
    args_string = self._format_args(action, default)
    if not args_string:
        parts.extend(action.option_strings)
    else:
        for option_string in action.option_strings:
            parts.append('%s %s' % (option_string, args_string))

or is there a way to incorporate the existing value string append parts.append('%s %s' % (option_string, args_string)) into the extend() as well?

Sorry if this is a basic question.

[remilapeyre]

Hi @csabella, I'm looking at old PRs and this one seems still relevant as thee bug is still present on master. I think parts.extend(action.option_strings) was what Serhiy Stochaka meant by replacing the loop with extend().