Patchwork mq: refactor usage of repo.branchmap().iteritems() with itervalues()

login
register
mail settings
Submitter Brodie Rao
Date Sept. 25, 2013, 9:38 p.m.
Message ID <5ddfc6e253794f76691b.1380145136@kapp.staff.sf.atlassian.com>
Download mbox | patch
Permalink /patch/2639/
State Accepted
Commit fb583a1efef04dcb1c0326622661d9f959fec317
Headers show

Comments

Brodie Rao - Sept. 25, 2013, 9:38 p.m.
# HG changeset patch
# User Brodie Rao <brodie@sf.io>
# Date 1364871676 25200
#      Mon Apr 01 20:01:16 2013 -0700
# Node ID 5ddfc6e253794f76691b27f805b1275d54708298
# Parent  50d721553198cea51c30f53b76d41dc919280097
mq: refactor usage of repo.branchmap().iteritems() with itervalues()
Brodie Rao - Sept. 25, 2013, 10:43 p.m.
On Wed, Sep 25, 2013 at 2:49 PM, Kevin Bullock
<kbullock+mercurial@ringworld.org> wrote:
> On 25 Sep 2013, at 4:38 PM, Brodie Rao wrote:
>
>> # HG changeset patch
>> # User Brodie Rao <brodie@sf.io>
>> # Date 1364871676 25200
>> #      Mon Apr 01 20:01:16 2013 -0700
>> # Node ID 5ddfc6e253794f76691b27f805b1275d54708298
>> # Parent  50d721553198cea51c30f53b76d41dc919280097
>> mq: refactor usage of repo.branchmap().iteritems() with itervalues()
>>
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -1222,9 +1222,7 @@ class queue(object):
>>         diffopts = self.diffopts()
>>         wlock = repo.wlock()
>>         try:
>> -            heads = []
>> -            for b, ls in repo.branchmap().iteritems():
>> -                heads += ls
>> +            heads = [h for hs in repo.branchmap().itervalues() for h in hs]
>
> The double-`for` is really confusing to me here.

If you insert some imaginary punctuation, it might make more sense:

[h <- for hs in repo.branchmap().itervalues(): for h in hs:]

That said, I just realized this could theoretically make the code
microscopically slower:

>>> timeit.timeit('things = [n for ns in stuff.itervalues() for n in ns]', 'stuff = {n: range(10 * n) for n in range(10)}') / 1000000.0
2.6555580139160157e-05

>>> timeit.timeit('things = []\nfor ns in stuff.itervalues():\n    things += ns\n', 'stuff = {n: range(10 * n) for n in range(10)}') / 1000000.0
4.299727916717529e-06

I guess the latter is faster because the final list ends up doing less
resizing as it's added to. I'm not sure if the performance difference
really matters though.
Augie Fackler - Sept. 26, 2013, 1:57 p.m.
On Sep 25, 2013, at 5:49 PM, Kevin Bullock <kbullock+mercurial@ringworld.org> wrote:

> On 25 Sep 2013, at 4:38 PM, Brodie Rao wrote:
> 
>> # HG changeset patch
>> # User Brodie Rao <brodie@sf.io>
>> # Date 1364871676 25200
>> #      Mon Apr 01 20:01:16 2013 -0700
>> # Node ID 5ddfc6e253794f76691b27f805b1275d54708298
>> # Parent  50d721553198cea51c30f53b76d41dc919280097
>> mq: refactor usage of repo.branchmap().iteritems() with itervalues()
>> 
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -1222,9 +1222,7 @@ class queue(object):
>>        diffopts = self.diffopts()
>>        wlock = repo.wlock()
>>        try:
>> -            heads = []
>> -            for b, ls in repo.branchmap().iteritems():
>> -                heads += ls
>> +            heads = [h for hs in repo.branchmap().itervalues() for h in hs]
> 
> The double-`for` is really confusing to me here.

Agreed. At /least/ parenthesize the nested generator expression. Or something. This is mq, perf isn't exactly critical.
Brodie Rao - Sept. 26, 2013, 5:28 p.m.
On Thu, Sep 26, 2013 at 6:57 AM, Augie Fackler <raf@durin42.com> wrote:
>
> On Sep 25, 2013, at 5:49 PM, Kevin Bullock
> <kbullock+mercurial@ringworld.org> wrote:
>
> On 25 Sep 2013, at 4:38 PM, Brodie Rao wrote:
>
> # HG changeset patch
> # User Brodie Rao <brodie@sf.io>
> # Date 1364871676 25200
> #      Mon Apr 01 20:01:16 2013 -0700
> # Node ID 5ddfc6e253794f76691b27f805b1275d54708298
> # Parent  50d721553198cea51c30f53b76d41dc919280097
> mq: refactor usage of repo.branchmap().iteritems() with itervalues()
>
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -1222,9 +1222,7 @@ class queue(object):
>        diffopts = self.diffopts()
>        wlock = repo.wlock()
>        try:
> -            heads = []
> -            for b, ls in repo.branchmap().iteritems():
> -                heads += ls
> +            heads = [h for hs in repo.branchmap().itervalues() for h in hs]
>
>
> The double-`for` is really confusing to me here.
>
>
> Agreed. At /least/ parenthesize the nested generator expression. Or
> something. This is mq, perf isn't exactly critical.

There's no nested generator expression (or nested anything). This is a
single list comprehension that has two loops. The list is built from
the results of the inner loop. If you're not familiar with this
syntax, you can read more about it here:
http://docs.python.org/2/tutorial/datastructures.html#list-comprehensions

We do this elsewhere in the code base. There's no parentheses that you
could add to this without introducing syntax errors.
Martin Geisler - Sept. 26, 2013, 9:34 p.m.
Kevin Bullock <kbullock+mercurial@ringworld.org> writes:

> On 25 Sep 2013, at 4:38 PM, Brodie Rao wrote:
>
>> # HG changeset patch
>> # User Brodie Rao <brodie@sf.io>
>> # Date 1364871676 25200
>> #      Mon Apr 01 20:01:16 2013 -0700
>> # Node ID 5ddfc6e253794f76691b27f805b1275d54708298
>> # Parent  50d721553198cea51c30f53b76d41dc919280097
>> mq: refactor usage of repo.branchmap().iteritems() with itervalues()
>> 
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -1222,9 +1222,7 @@ class queue(object):
>>         diffopts = self.diffopts()
>>         wlock = repo.wlock()
>>         try:
>> -            heads = []
>> -            for b, ls in repo.branchmap().iteritems():
>> -                heads += ls
>> +            heads = [h for hs in repo.branchmap().itervalues() for h in hs]
>
> The double-`for` is really confusing to me here.

Yeah, it's a slightly unusual construct.

I would prefer a loop over itervalues (that's a good change) and then
use extend instead +=. I find extend better because it has no return
value and no return value is needed or wanted here.
Matt Mackall - Oct. 19, 2013, 10:58 p.m.
On Wed, 2013-09-25 at 14:38 -0700, Brodie Rao wrote:
> # HG changeset patch
> # User Brodie Rao <brodie@sf.io>
> # Date 1364871676 25200
> #      Mon Apr 01 20:01:16 2013 -0700
> # Node ID 5ddfc6e253794f76691b27f805b1275d54708298
> # Parent  50d721553198cea51c30f53b76d41dc919280097
> mq: refactor usage of repo.branchmap().iteritems() with itervalues()

Queued for default, thanks. Follow-ups to make the double-for less
confusing welcome, but this is arguably colloquial Python.

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -1222,9 +1222,7 @@  class queue(object):
         diffopts = self.diffopts()
         wlock = repo.wlock()
         try:
-            heads = []
-            for b, ls in repo.branchmap().iteritems():
-                heads += ls
+            heads = [h for hs in repo.branchmap().itervalues() for h in hs]
             if not heads:
                 heads = [nullid]
             if repo.dirstate.p1() not in heads and not exact: