[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