| Message ID | 1409303222-19093-1-git-send-email-rep.dot.nop@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 2014-08-29 at 11:07 +0200, Bernhard Reutner-Fischer wrote: > Allow commands that take an ID to operate on multiple IDs. > E.g.: > update -s Superseded 1 2 3 4 5 > apply 2 4 6 > > Reject update -c COMMIT-REF on multiple IDs though as that does not > make sense. > > Implementation note: > nargs='*' instead of '?' results in (wrong/inconvenient): > mutually exclusive arguments must be optional > So remove mutual exclusive handling via argparse and instead do it by > hand. This might be implemented more conveniently in later python but we > (have to) stick with 2.7.x for the time being. > > Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> > --- > apps/patchwork/bin/pwclient | 60 +++++++++++++++++++++++++-------------------- > 1 file changed, 34 insertions(+), 26 deletions(-) > > diff --git a/apps/patchwork/bin/pwclient b/apps/patchwork/bin/pwclient > index a7afafc..bd79b3a 100755 > --- a/apps/patchwork/bin/pwclient > +++ b/apps/patchwork/bin/pwclient > @@ -331,13 +331,12 @@ class _RecursiveHelpAction(argparse._HelpAction): > > def main(): > hash_parser = argparse.ArgumentParser(add_help=False, version=False) > - hash_parser_x = hash_parser.add_mutually_exclusive_group(required=True) > - hash_parser_x.add_argument( > + hash_parser.add_argument( > '-h', metavar='HASH', dest='hash', action='store', required=False, > help='''Lookup by patch hash''' > ) > - hash_parser_x.add_argument( > - 'id', metavar='ID', nargs='?', action='store', type=int, > + hash_parser.add_argument( > + 'id', metavar='ID', nargs='*', action='store', type=int, > help='Patch ID', > ) > > @@ -377,7 +376,7 @@ def main(): > add_help=False, > version=False, > formatter_class=argparse.RawDescriptionHelpFormatter, > - epilog='''(apply | get | info | view | update) (-h HASH | ID)''', > + epilog='''(apply | get | info | view | update) (-h HASH | ID [ID ...])''', > ) > action_parser.add_argument( > '--help', > @@ -440,7 +439,8 @@ def main(): > update_parser = subparsers.add_parser( > 'update', parents=[hash_parser], > add_help=False, > - help='''Update patch''' > + help='''Update patch''', > + epilog='''Using a COMMIT-REF allows for only one ID to be specified''', > ) > update_parser.set_defaults(subcmd='update') > update_parser.add_argument( > @@ -475,6 +475,12 @@ def main(): > args = action_parser.parse_args() > args=dict(vars(args)) > > + if args.get('hash') and len(args.get('id')): > + # mimic mutual exclusive group > + sys.stderr.write("[-h HASH] and [ID [ID ...]] are mutually exlusive!\n") > + action_parser.print_help() > + sys.exit(1) > + > # set defaults > filt = Filter() > submitter_str = "" > @@ -484,7 +490,7 @@ def main(): > state_str = "" > hash_str = None > msgid_str = "" > - id_str = None > + patch_ids = None > url = DEFAULT_URL > > action = args.get('subcmd') > @@ -497,12 +503,17 @@ def main(): > submitter_str = args.get('w') > if args.get('d'): > delegate_str = args.get('d') > - if args.get('c'): > - commit_str = args.get('c') > if args.get('hash'): > hash_str = args.get('hash') > if args.get('id'): > - id_str = args.get('id') > + patch_ids = frozenset(args.get('id')) > + if args.get('c'): > + # update multiple IDs with a single commit-hash does not make sense > + if action == 'update' and patch_ids and len(patch_ids) > 1: > + sys.stderr.write("Declining update with COMMIT-REF on multiple IDs\n") > + update_parser.print_help() > + sys.exit(1) > + commit_str = args.get('c') > if args.get('m'): > msgid_str = args.get('m') > if args.get('n') != None: > @@ -603,13 +614,9 @@ def main(): > sys.stderr.write("Unable to connect to %s\n" % url) > sys.exit(1) > > - patch_id = None > - # hash_str and id_str are mutually exclusive > - if hash_str: > - patch_id = patch_id_from_hash(rpc, project_str, hash_str) > - else: > - # id_str from argparse is an int > - patch_id = id_str > + # It should be safe to assume hash_str is not zero, but who knows.. > + if hash_str != None: > + patch_ids = [patch_id_from_hash(rpc, project_str, hash_str)] > > if action == 'list' or action == 'search': > if args.get('patch_name') != None: > @@ -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 ) ? 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... Obviously this applies to all the actions you converted to arrays... Thanks, Jake > else: > sys.stderr.write("Unknown action '%s'\n" % action)
[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,
diff --git a/apps/patchwork/bin/pwclient b/apps/patchwork/bin/pwclient index a7afafc..bd79b3a 100755 --- a/apps/patchwork/bin/pwclient +++ b/apps/patchwork/bin/pwclient @@ -331,13 +331,12 @@ class _RecursiveHelpAction(argparse._HelpAction): def main(): hash_parser = argparse.ArgumentParser(add_help=False, version=False) - hash_parser_x = hash_parser.add_mutually_exclusive_group(required=True) - hash_parser_x.add_argument( + hash_parser.add_argument( '-h', metavar='HASH', dest='hash', action='store', required=False, help='''Lookup by patch hash''' ) - hash_parser_x.add_argument( - 'id', metavar='ID', nargs='?', action='store', type=int, + hash_parser.add_argument( + 'id', metavar='ID', nargs='*', action='store', type=int, help='Patch ID', ) @@ -377,7 +376,7 @@ def main(): add_help=False, version=False, formatter_class=argparse.RawDescriptionHelpFormatter, - epilog='''(apply | get | info | view | update) (-h HASH | ID)''', + epilog='''(apply | get | info | view | update) (-h HASH | ID [ID ...])''', ) action_parser.add_argument( '--help', @@ -440,7 +439,8 @@ def main(): update_parser = subparsers.add_parser( 'update', parents=[hash_parser], add_help=False, - help='''Update patch''' + help='''Update patch''', + epilog='''Using a COMMIT-REF allows for only one ID to be specified''', ) update_parser.set_defaults(subcmd='update') update_parser.add_argument( @@ -475,6 +475,12 @@ def main(): args = action_parser.parse_args() args=dict(vars(args)) + if args.get('hash') and len(args.get('id')): + # mimic mutual exclusive group + sys.stderr.write("[-h HASH] and [ID [ID ...]] are mutually exlusive!\n") + action_parser.print_help() + sys.exit(1) + # set defaults filt = Filter() submitter_str = "" @@ -484,7 +490,7 @@ def main(): state_str = "" hash_str = None msgid_str = "" - id_str = None + patch_ids = None url = DEFAULT_URL action = args.get('subcmd') @@ -497,12 +503,17 @@ def main(): submitter_str = args.get('w') if args.get('d'): delegate_str = args.get('d') - if args.get('c'): - commit_str = args.get('c') if args.get('hash'): hash_str = args.get('hash') if args.get('id'): - id_str = args.get('id') + patch_ids = frozenset(args.get('id')) + if args.get('c'): + # update multiple IDs with a single commit-hash does not make sense + if action == 'update' and patch_ids and len(patch_ids) > 1: + sys.stderr.write("Declining update with COMMIT-REF on multiple IDs\n") + update_parser.print_help() + sys.exit(1) + commit_str = args.get('c') if args.get('m'): msgid_str = args.get('m') if args.get('n') != None: @@ -603,13 +614,9 @@ def main(): sys.stderr.write("Unable to connect to %s\n" % url) sys.exit(1) - patch_id = None - # hash_str and id_str are mutually exclusive - if hash_str: - patch_id = patch_id_from_hash(rpc, project_str, hash_str) - else: - # id_str from argparse is an int - patch_id = id_str + # It should be safe to assume hash_str is not zero, but who knows.. + if hash_str != None: + patch_ids = [patch_id_from_hash(rpc, project_str, hash_str)] if action == 'list' or action == 'search': if args.get('patch_name') != None: @@ -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] else: sys.stderr.write("Unknown action '%s'\n" % action)
Allow commands that take an ID to operate on multiple IDs. E.g.: update -s Superseded 1 2 3 4 5 apply 2 4 6 Reject update -c COMMIT-REF on multiple IDs though as that does not make sense. Implementation note: nargs='*' instead of '?' results in (wrong/inconvenient): mutually exclusive arguments must be optional So remove mutual exclusive handling via argparse and instead do it by hand. This might be implemented more conveniently in later python but we (have to) stick with 2.7.x for the time being. Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> --- apps/patchwork/bin/pwclient | 60 +++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 26 deletions(-)