[PATCH 5/n] pwclient: allow multiple IDs

[Bernhard Reutner-Fischer]

[Please keep me in CC..]

On Fri, Aug 29, 2014 at 03:11:34PM +0000, Keller, Jacob E wrote:
>On Fri, 2014-08-29 at 11:07 +0200, Bernhard Reutner-Fischer wrote:

>> @@ -623,28 +630,29 @@ def main():
>>          action_states(rpc)
>>  
>>      elif action == 'view':
>> -        s = rpc.patch_get_mbox(patch_id)
>> -        if len(s) > 0:
>> -            print unicode(s).encode("utf-8")
>> +        for patch_id in patch_ids:
>> +            s = rpc.patch_get_mbox(patch_id)
>> +            if len(s) > 0:
>> +                print unicode(s).encode("utf-8")
>>  
>>      elif action in ('get', 'save', 'info'):
>>          if action == 'info':
>> -            action_info(rpc, patch_id)
>> +            [action_info(rpc, patch_id) for patch_id in patch_ids]
>>          else:
>> -            action_get(rpc, patch_id)
>> +            [action_get(rpc, patch_id) for patch_id in patch_ids]
>>  
>>      elif action == 'apply':
>> -        action_apply(rpc, patch_id)
>> +        [action_apply(rpc, patch_id) for patch_id in patch_ids]
>>  
>>      elif action == 'git-am':
>>          cmd = ['git', 'am']
>>          if do_signoff:
>>              cmd.append('-s')
>> -        action_apply(rpc, patch_id, cmd)
>> +        [action_apply(rpc, patch_id, cmd) for patch_id in patch_ids]
>>  
>>      elif action == 'update':
>> -        action_update_patch(rpc, patch_id, state = state_str,
>> -                commit = commit_str)
>> +        [action_update_patch(rpc, patch_id, state = state_str,
>> +                commit = commit_str) for patch_id in patch_ids]
>>  
>
>Since you aren't saving the array that you generate here, wouldn't it
>make more sense to just do:
>
>for patch_id in patch_ids:
>	action_update_patch(rpc, patch_id, state = state_str,
>		commit = commit_str )
>
>
>?

That's a style question i assume. I don't mind either way, please
follow-up if you prefer the for-loop variant.

>
>I don't see why you bother to actually create the array, (obviously you
>can't just use a generator expression since that won't be evaluated. I
>don't think you need to worry about catching the whole array here either
>if I understand correctly.
>
>I am not sure if Python will be smart enough to avoid creating the array
>in this case, though it probably doesn't really hurt anything...

One could argue that if python really builds a list then python should
be fixed ;)

But let's not speculate but simply see what it makes out of what i hastily
scribbled above:

$ cat sample.py
import dis

def consume(patch_id):
  """consume the arg, return nothing"""

def arr(patch_ids):
  [consume(patch_id) for patch_id in patch_ids]
def loop(patch_ids):
  for patch_id in patch_ids:
    consume(patch_id)

print("\n# arr")
dis.dis(arr)
print("\n# loop")
dis.dis(loop)

$ python2.7 ./sample.py 

# arr
  7           0 BUILD_LIST               0
              3 LOAD_FAST                0 (patch_ids)
              6 GET_ITER            
        >>    7 FOR_ITER                18 (to 28)
             10 STORE_FAST               1 (patch_id)
             13 LOAD_GLOBAL              0 (consume)
             16 LOAD_FAST                1 (patch_id)
             19 CALL_FUNCTION            1
             22 LIST_APPEND              2
             25 JUMP_ABSOLUTE            7
        >>   28 POP_TOP             
             29 LOAD_CONST               0 (None)
             32 RETURN_VALUE        

# loop
  9           0 SETUP_LOOP              24 (to 27)
              3 LOAD_FAST                0 (patch_ids)
              6 GET_ITER            
        >>    7 FOR_ITER                16 (to 26)
             10 STORE_FAST               1 (patch_id)

 10          13 LOAD_GLOBAL              0 (consume)
             16 LOAD_FAST                1 (patch_id)
             19 CALL_FUNCTION            1
             22 POP_TOP             
             23 JUMP_ABSOLUTE            7
        >>   26 POP_BLOCK           
        >>   27 LOAD_CONST               0 (None)
             30 RETURN_VALUE        

$ python3.4 ./sample.py 

# arr
  7           0 LOAD_CONST               1 (<code object <listcomp> at 0x7f4d1df55030, file "./sample.py", line 7>)
              3 LOAD_CONST               2 ('arr.<locals>.<listcomp>')
              6 MAKE_FUNCTION            0
              9 LOAD_FAST                0 (patch_ids)
             12 GET_ITER
             13 CALL_FUNCTION            1 (1 positional, 0 keyword pair)
             16 POP_TOP
             17 LOAD_CONST               0 (None)
             20 RETURN_VALUE

# loop
  9           0 SETUP_LOOP              24 (to 27)
              3 LOAD_FAST                0 (patch_ids)
              6 GET_ITER
        >>    7 FOR_ITER                16 (to 26)
             10 STORE_FAST               1 (patch_id)

 10          13 LOAD_GLOBAL              0 (consume)
             16 LOAD_FAST                1 (patch_id)
             19 CALL_FUNCTION            1 (1 positional, 0 keyword pair)
             22 POP_TOP
             23 JUMP_ABSOLUTE            7
        >>   26 POP_BLOCK
        >>   27 LOAD_CONST               0 (None)
             30 RETURN_VALUE

So, if this really bothers you then please tweak it to use a for-loop.
I do not think that it makes a terrible lot of difference for the
use-case at hand though, performance-wise or memory-wise.
As said, if you dislike the coding style then please follow-up.

TIA && cheers,