[PATCH 6/n] pwclient: diagnose hash_parser errors gracefully

[Keller, Jacob E]

> -----Original Message-----
> From: Bernhard Reutner-Fischer [mailto:rep.dot.nop at gmail.com]
> Sent: Monday, September 08, 2014 4:23 AM
> To: Keller, Jacob E
> Cc: patchwork at lists.ozlabs.org
> Subject: Re: [PATCH 6/n] pwclient: diagnose hash_parser errors
> gracefully
> 
> [Please keep me CC'ed]
> 
> On Fri, Aug 29, 2014 at 03:14:35PM +0000, Keller, Jacob E wrote:
> >On Fri, 2014-08-29 at 14:02 +0200, Bernhard Reutner-Fischer wrote:
> 
> >> @@ -618,6 +620,23 @@ def main():
> >>      if hash_str != None:
> >>          patch_ids = [patch_id_from_hash(rpc, project_str, hash_str)]
> >>
> >> +    # helper for non_empty() to print correct helptext
> >> +    h = None
> >> +    try:
> >> +        h = locals()[action + '_parser']
> >> +    except:
> >> +        pass # never happens
> >
> >This is bad. Why blindly pass any exceptions here?? If you "know" it
> >never happens then you shouldn't have an except block, since that will
> >prevent debugging real exceptions that could occur. Unless you are
> >specifically blocking some exception you know will happen? but then it
> >should be typed to only catch the ones you know of.......
> 
> Iff we update to an argparse that supports aliases and we do not
> deprecate action == "search" as an alias for "list" then we might end up
> with no search_parser here. As that is not a real show-stopper, i did
> choose to just not print the corresponding parser's help-text if there
> is no such parser. Note that this currently cannot happen since we
> cannot use aliases with argparse from python-2.7 (AFAICS).
> 
> thanks,
> >

That makes sense :)

Regards,
Jake

> >> +    # Require either hash_str or IDs for
> >> +    def non_empty(h, patch_ids):
> >> +        """Error out if no patch IDs were specified"""
> >> +        if patch_ids == None or len(patch_ids) < 1:
> >> +            sys.stderr.write("Error: Missing Argument! " +
> >> +              "Either [-h HASH] or [ID [ID ...]] are required\n")
> >> +            if h:
> >> +                h.print_help()
> >> +            sys.exit(1)
> >> +        return patch_ids
> >> +
> >>      if action == 'list' or action == 'search':
> >>          if args.get('patch_name') != None:
> >>              filt.add("name__icontains", args.get('patch_name'))
> >> @@ -630,29 +649,31 @@ def main():
> >>          action_states(rpc)
> >>
> >>      elif action == 'view':
> >> -        for patch_id in patch_ids:
> >> +        for patch_id in non_empty(h, patch_ids):
> >>              s = rpc.patch_get_mbox(patch_id)
> >>              if len(s) > 0:
> >>                  print unicode(s).encode("utf-8")