A small patch...

1 view
Skip to first unread message

Fernando Perez

unread,
Jan 5, 2010, 2:27:48 AM1/5/10
to argparse-devs
Hi Steven,

I'm attaching a small patch against current SVN of argparse, that
makes three little changes, for your consideration. I'm happy to
break it up in three if you prefer, but it's so small and simple I
figured a single patch would be OK.

What it does:

1. Small optimization in the constructor:

- for name in kwargs:
- setattr(self, name, kwargs[name])
+ self.__dict__.update(**kwargs)

I think this is safe, and faster as it relies on a single call to
python's dict .update() method rather than manually looping over keys.

2. Added support for 'key in namespace'

+ def __contains__(self, key):
+ return key in self.__dict__

This allows the following:

In [2]: ns = argparse.Namespace(x=1,y=2)

In [3]: 'x' in ns
Out[3]: True

In [4]: 'z' in ns
Out[4]: False

I find it much more natural to say 'if some_option in namespace' than
'if hasattr(namespace, some_option)'.

3. Directing user-intended help/usage messages to stdout by default:

@@ -2288,9 +2290,13 @@
# Help-printing methods
# =====================
def print_usage(self, file=None):
+ if file is None:
+ file = _sys.stdout
self._print_message(self.format_usage(), file)

def print_help(self, file=None):
+ if file is None:
+ file = _sys.stdout

The reason for this is that printing to stderr by default makes it
harder for users to page large help messages (like IPython's). Many
users don't know what the shell syntax for piping stderr is, they only
know

foo --help | more

and that doesn't work. Git and many other programs follow the
practice of printing user help screens to stdout instead of stderr for
this very reason, so I hope you'll consider the change.

Please let me know (I've signed up to the group) if you agree with
these changes, as for now I'm carrying them in IPython's copy of
argparse, but ultimately I don't want our copy to diverge at all from
yours. So if for some reason you don't want these, we'll monkeypatch
instead of touching the sources. If you agree with them, great, we'll
already be using them :)

Best regards, and thanks again for the great tool!

f

argparse.diff

Steven Bethard

unread,
Jan 7, 2010, 12:47:58 PM1/7/10
to argpar...@googlegroups.com
On Mon, Jan 4, 2010 at 11:27 PM, Fernando Perez <fpere...@gmail.com> wrote:
> I'm attaching a small patch against current SVN of argparse, that
> makes three little changes, for your consideration.  I'm happy to
> break it up in three if you prefer, but it's so small and simple I
> figured a single patch would be OK.
>
> What it does:
>
> 1. Small optimization in the constructor:
>
> -        for name in kwargs:
> -            setattr(self, name, kwargs[name])
> +        self.__dict__.update(**kwargs)
>
> I think this is safe, and faster as it relies on a single call to
> python's dict .update() method rather than manually looping over keys.

The one thing this stops is subclasses from overriding __setattr__ to
catch updates. I don't know how important that is, but I'm also not
sure how important the speed of the constructor is. Have you run into
problems with it?

> 2. Added support for 'key in namespace'
>
> +    def __contains__(self, key):
> +        return key in self.__dict__
>
> This allows the following:
>
> In [2]: ns = argparse.Namespace(x=1,y=2)
>
> In [3]: 'x' in ns
> Out[3]: True
>
> In [4]: 'z' in ns
> Out[4]: False
>
> I find it much more natural to say 'if some_option in namespace' than
> 'if hasattr(namespace, some_option)'.

This looks fine, though it will need some tests as well.

> 3. Directing user-intended help/usage messages to stdout by default:
>
> @@ -2288,9 +2290,13 @@
>     # Help-printing methods
>     # =====================
>     def print_usage(self, file=None):
> +        if file is None:
> +            file = _sys.stdout
>         self._print_message(self.format_usage(), file)
>
>     def print_help(self, file=None):
> +        if file is None:
> +            file = _sys.stdout
>
> The reason for this is that printing to stderr by default makes it
> harder for users to page large help messages (like IPython's).  Many
> users don't know what the shell syntax for piping stderr is, they only
> know
>
> foo --help | more
>
> and that doesn't work.  Git and many other programs follow the
> practice of printing user help screens to stdout instead of stderr for
> this very reason, so I hope you'll consider the change.

Yep, I checked around and I agree that most other programs send help
messages to stdout. This behavior was inherited from optparse, but I
think you're right that usage and help (though not version, looking at
some other programs, like Java) should go to stdout. It's possbile
this will break some tests, but I'm willing to consider that a bug in
the test.

Steve
--
Where did you get that preposterous hypothesis?
Did Steve tell you that?
--- The Hiphopopotamus

Fernando Perez

unread,
Jan 7, 2010, 3:24:35 PM1/7/10
to argparse-devs
Hi Steven,

thanks for considering this. I've attached an updated version of the
patch (against updated SVN from a minute ago), comments below.

On Thu, Jan 7, 2010 at 9:47 AM, Steven Bethard <steven....@gmail.com> wrote:
> On Mon, Jan 4, 2010 at 11:27 PM, Fernando Perez <fpere...@gmail.com> wrote:

>> 1. Small optimization in the constructor:
>>
>> -        for name in kwargs:
>> -            setattr(self, name, kwargs[name])
>> +        self.__dict__.update(**kwargs)
>>
>> I think this is safe, and faster as it relies on a single call to
>> python's dict .update() method rather than manually looping over keys.
>
> The one thing this stops is subclasses from overriding __setattr__ to
> catch updates. I don't know how important that is, but I'm also not
> sure how important the speed of the constructor is. Have you run into
> problems with it?

Not directly, but I have a general policy of trying to keep python
code as tight as is reasonable, because things are slow enough as they
are already :) To clarify where I'm coming from (this isn't just
early-optimizer-madness): over the years IPython has become
significantly slower to start up than a 'bare' python, and it's really
not easily optimizable anymore. We don't have one single bottleneck
anywhere, it's just that ipython loads a lot of stuff and all those
little chunks of 5-10 milliseconds add up at the end of the day.
Because of this, my reflexes have become very tuned to using the most
efficient idioms of the language when I can, and to look for builtin
structures and syntax to do the work for me (sets, dicts, listcomps)
whenever possible in favor of manual looping, since I know that will
typically be faster.

In this particular case, I'd be willing to pay the price of a small
loss of flexibility, because it only changes the constructor.
Subclasses that override __setattr__ to monitor updates would still
behave as expected once the object has been created, they just
wouldn't react to individual assignments at construction time.

Having said all this, of the whole patch, this is the part I care the
least about. I just wanted to clarify why I wrote that, but it's
fairly secondary to me and your concern may be more important in
practice for real-world argparse use than I'm seeing. So if you
decide to drop this part, I won't argue for it further. But thanks
for the point you brought up, I actually hadn't thought of it
initially, so I learned something :)

The attached patch still has this, but I can either re-make it
trivially or you can drop it out, just let me know.

>> 2. Added support for 'key in namespace'

[...]


>> I find it much more natural to say 'if some_option in namespace' than
>> 'if hasattr(namespace, some_option)'.
>
> This looks fine, though it will need some tests as well.

OK, I put in some simple tests in the patch included. I verified they
run correctly, though they are very basic. If you can suggest better
ones I'm happy to add more, it's just that for such a simple feature I
couldn't think quickly of much more to test.

>> 3. Directing user-intended help/usage messages to stdout by default:

> Yep, I checked around and I agree that most other programs send help


> messages to stdout. This behavior was inherited from optparse, but I
> think you're right that usage and help (though not version, looking at
> some other programs, like Java) should go to stdout. It's possbile
> this will break some tests, but I'm willing to consider that a bug in
> the test.

Thanks! I should add that in making my 'x in ns' tests above, I
temporarily disabled this, because I see that with it, your test suite
does fail all over the place. But I got a little lost in how you
manage stdout/err for help/versions, so for now I haven't updated the
whole test suite to pass with this change. I hope if you like this
idea, you'll be able to do it in 1/10th the time it would take me, as
you know your code better than I do.

Let me know if I can do anything else to help with this.

Cheers,

f

argparse2.diff

Steven Bethard

unread,
Feb 28, 2010, 12:28:48 AM2/28/10
to argpar...@googlegroups.com
Sorry it took so long to get to this. Real life is killing me right now.

On Thu, Jan 7, 2010 at 12:24 PM, Fernando Perez <fpere...@gmail.com> wrote:
> On Thu, Jan 7, 2010 at 9:47 AM, Steven Bethard <steven....@gmail.com> wrote:
>> On Mon, Jan 4, 2010 at 11:27 PM, Fernando Perez <fpere...@gmail.com> wrote:
>
>>> 1. Small optimization in the constructor:
>>>
>>> -        for name in kwargs:
>>> -            setattr(self, name, kwargs[name])
>>> +        self.__dict__.update(**kwargs)
>>>
>>> I think this is safe, and faster as it relies on a single call to
>>> python's dict .update() method rather than manually looping over keys.
>>
>> The one thing this stops is subclasses from overriding __setattr__ to
>> catch updates. I don't know how important that is, but I'm also not
>> sure how important the speed of the constructor is. Have you run into
>> problems with it?
>
> Not directly, but I have a general policy of trying to keep python
> code as tight as is reasonable, because things are slow enough as they
> are already :)

Turns out that this version of the update method is not supported in
Python 2.3, which is the earliest version that argparse supports. (I
discovered this by applying your patch and running my usual set of
tests). So I can't make this change.

>>> 2. Added support for 'key in namespace'
> [...]
>>> I find it much more natural to say 'if some_option in namespace' than
>>> 'if hasattr(namespace, some_option)'.

>>> 3. Directing user-intended help/usage messages to stdout by default:


>
>> Yep, I checked around and I agree that most other programs send help
>> messages to stdout. This behavior was inherited from optparse, but I
>> think you're right that usage and help (though not version, looking at
>> some other programs, like Java) should go to stdout.

Both of these are fixed in r81.

Reply all
Reply to author
Forward
0 new messages