| Message ID | 1445038743-50857-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Stephen Finucane |
| Headers | show |
Reviewed-by: Mike Frysinger <vapier@chromium.org>
-mike
On 16 Oct 2015 16:39, Brian Norris wrote:
> commit_str = args.get('c')
if you feel like cleaning things up, i find the args behavior weird and
non-standard. it does:
args = action_parser.parse_args()
args = dict(vars(args))
action = args.get('subcmd')
normally argparse code does:
args = action_parser.parse_args()
action = args.subcmd
i'm not sure why this dict/get style was picked. maybe Bernhard can
shed some light here.
-mike
On October 17, 2015 4:25:54 AM GMT+02:00, Mike Frysinger <vapier@gentoo.org> wrote: >On 16 Oct 2015 16:39, Brian Norris wrote: >> commit_str = args.get('c') > >if you feel like cleaning things up, i find the args behavior weird and >non-standard. it does: > args = action_parser.parse_args() > args = dict(vars(args)) > action = args.get('subcmd') > >normally argparse code does: > args = action_parser.parse_args() > action = args.subcmd > >i'm not sure why this dict/get style was picked. maybe Bernhard can >shed some light here. At this point all we need is a dict and I found it easier and shorter to type. I don't have a strong opinion about it, so if it's more pythonic to rephrase it, please do. I do remember to have gone through hoops to handle --help and --hash at the same time AND allowing for recursive printing of all help (I considered that a feature back then) but I do not remember if or why my attempt to use a conflict_handler didn't work out. Could be there was a bug in argparse back then or maybe I just goofed it. Thanks, >-mike
On 19 Oct 2015 09:57, Bernhard Reutner-Fischer wrote: > On October 17, 2015 4:25:54 AM GMT+02:00, Mike Frysinger wrote: > >On 16 Oct 2015 16:39, Brian Norris wrote: > >> commit_str = args.get('c') > > > >if you feel like cleaning things up, i find the args behavior weird and > >non-standard. it does: > > args = action_parser.parse_args() > > args = dict(vars(args)) > > action = args.get('subcmd') > > > >normally argparse code does: > > args = action_parser.parse_args() > > action = args.subcmd > > > >i'm not sure why this dict/get style was picked. maybe Bernhard can > >shed some light here. > > At this point all we need is a dict and I found it easier and shorter to type. i'm not sure how this is shorter: args.get('subcmd') than this: args.subcmd it's also more dangerous: get will not throw an error if the key doesn't exist. so if you did something like: args.get('submcd') you might not notice -- python would return None in the default case you didn't typo and everytime you typod. but if you did: args.submcd then python would an exception as soon as that code is run. since argparse has a default=None, args.subcmd would return None if the user hadn't specified anything. > I do remember to have gone through hoops to handle --help and --hash at the same time AND allowing for recursive printing of all help (I considered that a feature back then) but I do not remember if or why my attempt to use a conflict_handler didn't work out. Could be there was a bug in argparse back then or maybe I just goofed it. Brian has posted some CLs to clean that up by using some more of the argparse infrastructure. -mike
> This reduces the boilerplate we need and provides a more consistent help > output. e.g.: > > $ pwclient update -s FOO -c 1337 314159 1234567 > usage: pwclient update [--help] [-h HASH] [-p PROJECT] [-c COMMIT-REF] > [-s STATE] [-a {yes,no}] > [ID [ID ...]] > pwclient update: error: Declining update with COMMIT-REF on multiple IDs > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > v2: new in this series, per Mike's suggestion Yeah, this is a good improvement. Reviewed-by: Stephen Finucane <stephen.finucane@intel.com>
> > This reduces the boilerplate we need and provides a more consistent help > > output. e.g.: > > > > $ pwclient update -s FOO -c 1337 314159 1234567 > > usage: pwclient update [--help] [-h HASH] [-p PROJECT] [-c COMMIT-REF] > > [-s STATE] [-a {yes,no}] > > [ID [ID ...]] > > pwclient update: error: Declining update with COMMIT-REF on multiple > IDs > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > --- > > v2: new in this series, per Mike's suggestion > > Yeah, this is a good improvement. > > Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> Merged.
On Mon, Oct 19, 2015 at 01:56:35PM -0400, Mike Frysinger wrote: > On 19 Oct 2015 09:57, Bernhard Reutner-Fischer wrote: > > On October 17, 2015 4:25:54 AM GMT+02:00, Mike Frysinger wrote: > > >On 16 Oct 2015 16:39, Brian Norris wrote: > > >> commit_str = args.get('c') > > > > > >if you feel like cleaning things up, i find the args behavior weird and > > >non-standard. it does: > > > args = action_parser.parse_args() > > > args = dict(vars(args)) > > > action = args.get('subcmd') > > > > > >normally argparse code does: > > > args = action_parser.parse_args() > > > action = args.subcmd I took a quick look at what it takes to switch. A few questions. > > >i'm not sure why this dict/get style was picked. maybe Bernhard can > > >shed some light here. > > > > At this point all we need is a dict and I found it easier and shorter to type. > > i'm not sure how this is shorter: > args.get('subcmd') > than this: > args.subcmd > > it's also more dangerous: get will not throw an error if the key > doesn't exist. so if you did something like: > args.get('submcd') > you might not notice -- python would return None in the default case > you didn't typo and everytime you typod. but if you did: > args.submcd > then python would an exception as soon as that code is run. since > argparse has a default=None, args.subcmd would return None if the > user hadn't specified anything. Those benefits all seem nice to me. > > I do remember to have gone through hoops to handle --help and --hash > > at the same time AND allowing for recursive printing of all help (I > > considered that a feature back then) but I do not remember if or why > > my attempt to use a conflict_handler didn't work out. Could be there > > was a bug in argparse back then or maybe I just goofed it. > > Brian has posted some CLs to clean that up by using some more of the > argparse infrastructure. Right, so that part is fixed AFAICT. I wrote that stuff so long ago, I don't actually remember the details about the conflict_handler now :) Anyway, if we stop converting to a dict, then we have to be a little more careful on how we treat various arguments when implementing common code. e.g., we can't do this, since not all subcommands have the "archived" flag: archived_str = args.a ... if archived_str: # do something Maybe I'm not being creative (or pythonic) enough to figure out what the elegant solution would be that doesn't involve accidentally referencing attributes that only exist on some subcommands. Do we just go with hasattr()? Or getattr()? (But then we're still back to the same problem of typos...) Brian
diff --git a/patchwork/bin/pwclient b/patchwork/bin/pwclient index 9d34d541f3a8..76dd97b969ec 100755 --- a/patchwork/bin/pwclient +++ b/patchwork/bin/pwclient @@ -493,10 +493,8 @@ def main(): if args.get('hash') and len(args.get('id')): # mimic mutual exclusive group - sys.stderr.write("Error: [-h HASH] and [ID [ID ...]] " + - "are mutually exlusive\n") - locals()[action + '_parser'].print_help() - sys.exit(1) + locals()[action + '_parser'].error( + "[-h HASH] and [ID [ID ...]] are mutually exlusive") # set defaults filt = Filter() @@ -515,11 +513,7 @@ def main(): 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) + update_parser.error("Declining update with COMMIT-REF on multiple IDs") commit_str = args.get('c') if state_str is None and archived_str is None and action == 'update': @@ -529,9 +523,7 @@ def main(): try: filt.add("max_count", args.get('n')) except: - sys.stderr.write("Invalid maximum count '%s'\n" % args.get('n')) - action_parser.print_help() - sys.exit(1) + action_parser.error("Invalid maximum count '%s'" % args.get('n')) do_signoff = args.get('signoff') @@ -572,9 +564,7 @@ def main(): try: project_str = config.get('options', 'default') except: - sys.stderr.write("No default project configured in ~/.pwclientrc\n") - action_parser.print_help() - sys.exit(1) + action_parser.error("No default project configured in ~/.pwclientrc") if not config.has_section(project_str): sys.stderr.write('No section for project %s in ~/.pwclientrc\n' % project_str)
This reduces the boilerplate we need and provides a more consistent help output. e.g.: $ pwclient update -s FOO -c 1337 314159 1234567 usage: pwclient update [--help] [-h HASH] [-p PROJECT] [-c COMMIT-REF] [-s STATE] [-a {yes,no}] [ID [ID ...]] pwclient update: error: Declining update with COMMIT-REF on multiple IDs Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- v2: new in this series, per Mike's suggestion patchwork/bin/pwclient | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)