[PATCH 3/5] Raise exception instead of returning None

James Laska jlaska at redhat.com
Wed Dec 14 14:39:36 UTC 2011


On Wed, 2011-12-14 at 09:31 -0500, Chris Lalancette wrote:
> On Tue, Dec 13, 2011 at 11:11 AM, James Laska <jlaska at redhat.com> wrote:
> > ---
> >  oz/ozutil.py |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/oz/ozutil.py b/oz/ozutil.py
> > index 40932c0..727d7d2 100644
> > --- a/oz/ozutil.py
> > +++ b/oz/ozutil.py
> > @@ -265,9 +265,10 @@ def string_to_bool(instr):
> >     lower = instr.lower()
> >     if lower == 'no' or lower == 'false':
> >         return False
> > -    if lower == 'yes' or lower == 'true':
> > +    elif lower == 'yes' or lower == 'true':
> >         return True
> > -    return None
> > +    else:
> > +        raise Exception, "Unhandled input string: %s" % instr
> 
> The problem with doing this is that it makes the method less flexible,
> but more crucially it makes the error messages more generic.  If you
> look at all of the places in Oz that currently call string_to_bool(),
> they all check for None and raise an appropriate error.  If we move it
> into this function, we can remove some redundant lines of code at the
> expense of precision.  I'm inclined to leave things as-is, but I could
> be convinced by a good argument.

I'm with you on changing things as little as possible, which caused some
hesitation on sending this patch.

It was something that occurred to me while debugging the <persisted> vs
<persistent> issue.  I thought it could be confusing for the method to
return True/False or None.  Especially if the callers aren't
differentiating between False and None.  After further review, it seems
the callers of string_to_bool (config_get_boolean_key and
get_optional_repo_bool) already check for None.  

Based on that, your suggestion seems good to me :)

Thanks,
James

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/aeolus-devel/attachments/20111214/603946fa/attachment.sig>


More information about the aeolus-devel mailing list