New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve argparse.Namespace __repr__ for invalid identifiers. #68548
Comments
The argparse Namespace can be missleading in case where the args names are not valid identifiers, eg thinks like a closing bracket: In [5]: Namespace(a=1, **{')':3}) more funny: In [3]: Namespace(a=1, **{s:3}) for With this patch the args that are not valid identifiers are shown in ** unpacked-dict, which has the side effect of almost always having repr(eval(repr(obj)))== repr(obj). I'm sure we can find counter example with quotes and triple-quoted string... but anyway. with this patch (indentation mine for easy comparison): >>> from argparse import Namespace
>>> Namespace(a=1, **{')': 3})
Namespace(a=1, **{')': 3}) Which is I think what most user would expect. Test passes locally (except SSL cert, network thingies, curses and threaded_lru_cache) which look unrelated and is most likely due to my machine. |
LGTM. |
Off hand I don't see a problem with this patch (but I haven't tested it yet). But I have a couple of cautions: The docs say, regarding the Namespace class:
This patch improves the 'readable' part, but adds some complexity. The docs also note that the user can provide their own namespace object, and by implication, a custom Namespace class with this improved '__repr__'. The Namespace '__repr__' is mainly of value during code development, especially when trying ideas in an interactive shell. It's unlikely that you would want to show the whole namespace to your end user. So even if your final API requires funny characters, you don't need to use them during development. |
Sure and anyway if you have a huge namespace, things will become unreadable. One could also just use __repr(self)__ = repr(self.__dict__), that woudl be even simpler and readable :-) |
An alternative would be to wrap a non-identifier name in 'repr()': def repr1(self):
def fmt_name(name):
if name.isidentifier():
return name
else:
return repr(name)
type_name = type(self).__name__
arg_strings = []
for arg in self._get_args():
arg_strings.append(repr(arg))
for name, value in self._get_kwargs():
arg_strings.append('%s=%r' % (fmt_name(name), value))
return '%s(%s)' % (type_name, ', '.join(arg_strings)) This would produce lines like: Namespace(baz='one', 'foo bar'='test', 'x __y'='other')
Namespace(a=1, b=2, 'double " quote'='"', "single ' quote "="'")
With names that are deliberately messy, it is hard to say which is clearer. |
:-) I read that a I just feel like having a repr which is closer to the constructor signature is better, but I guess it's a question of taste. Anyway, both would be fine. |
I mentioned in the related bug/issue that no one has to use odd characters and spaces in the Namespace. While they are allowed by 'getattr' etc, the programmer has the option of supplying rational names in the 'dest' parameter. There's also the question of what kinds of strings can you supply via 'sys.argv'. For example, I have to use quotes to enter $ python echoargv.py '--b=2), Namespace(c' test Without them 'bash' gives me an error. Strings like this may be nice for exercising a patch, they may not be realistic in full argparse context. |
As far as I remember, argparse is not only use to parse things from sys.argv where the quoting is not necessary. And Namespace is not only use in argparse. But if you don't want improvement, feel free to close. |
If one is going to have a repr at all, I think it should be as accurate as practical. I think this is worthwhile, and favor the existing patch. |
New changeset dcc00d9ba8db by Berker Peksag in branch 'default': |
Thanks for the patch, Matthias. |
Thanks for accepting the patch. Looking forward to 3.6 ! :-) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: