<br><br><div class="gmail_quote">Pierre-Yves Chibon <<a href="mailto:pingou@pingoured.fr">pingou@pingoured.fr</a>> schrieb am Tue Dec 16 2014 at 2:52:33 PM:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Dec 16, 2014 at 02:41:53PM +0100, Pierre-Yves Chibon wrote:<br>
> On Tue, Dec 16, 2014 at 01:18:54PM +0000, Thomas Spura wrote:<br>
> > From 377c783cbb9460bba87fdda1add125<u></u>1bffe45b79 Mon Sep 17 00:00:00 2001<br>
> > From: Thomas Spura <<a href="mailto:thomas.spura@gmail.com" target="_blank">thomas.spura@gmail.com</a>><br>
> > Date: Tue, 16 Dec 2014 14:12:34 +0100<br>
> > Subject: [PATCH 2/2] Catch exception from FAS, when group does not exist<br>
> ><br>
> > Furthermore, the group_type must be 'pkgdb', otherwise pkgdb would<br>
> > throw an error later on.<br>
> > ---<br>
> > scripts/process-git-requests/<u></u>process-git-requests | 16 +++++++++-------<br>
> > 1 file changed, 9 insertions(+), 7 deletions(-)<br>
> ><br>
> > diff --git a/scripts/process-git-<u></u>requests/process-git-requests b/scripts/process-git-<u></u>requests/process-git-requests<br>
> > index bce4a54..99de338 100755<br>
> > --- a/scripts/process-git-<u></u>requests/process-git-requests<br>
> > +++ b/scripts/process-git-<u></u>requests/process-git-requests<br>
> > @@ -33,7 +33,7 @@ import time<br>
> > import webbrowser<br>
> > import socket<br>
> > from configobj import ConfigObj, flatten_errors<br>
> > -from fedora.client import AccountSystem, AuthError<br>
> > +from fedora.client import AccountSystem, AuthError, AppError<br>
> > from pkgdb2client import PkgDB, PkgDBException<br>
> > from optparse import OptionParser<br>
> > from validate import Validator<br>
> > @@ -507,12 +507,14 @@ def check_owner(fas, owner):<br>
> > if not group.endswith('-sig'):<br>
> > warnings.append('WARNING: "%s" is not a valid FAS group.' % group)<br>
> > return warnings<br>
> > - group = fas.group_by_name(group)<br>
> > - if not 'group_type' in group:<br>
> > - warnings.append('WARNING: "%s" is not a valid FAS account/group.' % owner)<br>
> > + try:<br>
> > + group = fas.group_by_name(group)<br>
> > + if not group['group_type'] == 'pkgdb':<br>
> > + warnings.append('WARNING: "%s" is not a valid FAS group.' % group)<br>
> > + return warnings<br>
> > + except AppError:<br>
> > + warnings.append('WARNING: "%s" is not a valid FAS group.' % group)<br>
> +1 for me otherwise,<br></blockquote><div><br></div><div>There is now a different warning message.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Actually, one more thing.<br>
<br>
If you<br>
- option a) move the check for the group type outside of the try/except (likely<br>
going to need something like group = None, try/except if group is not None:...<br>
<br>
or<br>
<br>
- option b) move more of the logic inside the try/except<br>
<br>
You can probably keep it so that the method as only 1 return statement :)<br></blockquote><div><br></div><div>I took option b, but initially avoided it, because I thought it was more readable back then ;)</div><div><br></div><div>So all changes are now melt into one commit now, see the attachment :)</div><div><br></div><div>Greetings,</div><div> Tom </div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But I could see other people disagreeing with this :)<br>
<br>
<br>
Pierre<br>
</blockquote></div>