[PATCH] Move Anaconda version detection from isys to Python code

Vratislav Podzimek vpodzime at redhat.com
Wed May 21 07:47:38 UTC 2014


On Tue, 2014-05-20 at 10:14 -0400, Anne Mulhern wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Tuesday, May 20, 2014 7:26:54 AM
> > Subject: Re: [PATCH] Move Anaconda version detection from isys to Python code
> > 
> > On Mon, 2014-05-19 at 14:35 -0400, Anne Mulhern wrote:
> > > It might be
> > > better to check for the existance of the version.py file, rather than
> > > whether
> > > or not an ImportError was raised on importing it, in determining the
> > > existance
> > > of the version since ImportErrors can be raised for lots of reasons. Also,
> > > I
> > > prefer None to 'unknown version'.
> > I prefer "uknown version". The function is supposed to return a string,
> > so it should return a string. Or it could raise an exception, but we
> > should stop misusing None this way.
> > 
> > --
> > Vratislav Podzimek
> > 
> > Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
> > 
> > 
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> > 
> 
> I don't understand what is meant by a misuse of None or by the statement
> that the function is supposed to return a string, so it should return a
> string.
> 
> O'Caml has an Option type with two constructors, None or Some.
> Scala has gone with something nearly identical.
> Haskell does the same thing, with constructors Nothing or Just for variety.
> Even Java now has an Optional type as of last September, with two methods, empty and of.
> The boost C++ libraries provide an optional class.
> All of these are examples of statically checked languages coping with the fact that
> sometimes a function has an actual value to return and sometimes it has nothing
> and nothing has its own special type.
That's the point -- a special type. In all of those languages (+ Vala,
Ada and others) the function's definition/signature tells you that the
returned value may be something like None. In Haskell, it's 'Maybe Int'
and the values are 'None' and 'Just 5' that are both clearly
distinguishable 1. from each other and 2. from '5' as an Int. And
similarly in the other langauges. 

> 
> My experience with dynamically checked languages is restricted to Python and
> some variants of Lisp. But I know that functions that return values of different
> types are extremely common in well-written Lisp programs. Returning #f when
> nothing is found is a commonplace idiom.
> 
> Python is tricky, because unlike in Lisp, None, False, [], "", {} are
> all false and it is harder to use the idiom:
> 
> val = func()
> if not val: #Maybe this is an empty result and maybe it's a non-result
> 
> and sometimes, when you actually need to tell the difference, you have to use:
> 
> if val is None: # definitely a non-result
> 
> In all the other languages I've mentioned returning some kind of invented
> value is considered a mistake because eventually
> some code will misinterpret the existence of that value as evidence
> that some value actually exists or that value will coincide with an actual value.
> I admit that it is unlikely that there will be any actual version of anaconda
> called "unknown version" but the principle is still there.
> Since none of these languages have type systems that require you to make this
> mistake, I prefer to avoid it.
I agree that returning an invented value is not a good practise, but in
this case, one can take "uknown version" as a valid value for version.

Having all [], "", None, set(),... evaluated to False is one of the
issues. The other one, and much bigger from my point of view and
experience, is the good old "NoneType has no attribute...". Being in the
installer team for more than 3 years I can honestly say that I've seen
at least 100 bugs like this. And here is a big difference:

"NoneType has no attribute something" is raised not from the place where
the problem/fail happened, but from a completely different place. The
traceback usually says nothing, you have to go to the sources or even
debugger and become Sherlock Holmes searching for a place and condition
where that None was born.

An exception, on the other hand, has it's full trace to the point, where
the problem/fail happened. You can see that in the traceback and you can
easily decide whether it is an acceptable case that can be handled
silently or if it is an error that should cause the program to stop and
request reporting a bug.

So by returning None in case of failures, the function hides quite a lot
of helpful information. Moreover, None is just a single value from which
you cannot determine the cause/reason of the failure. Whereas you can
have many types of exceptions or even a single type but with different
messages.

> 
> Given all this in my background, my reasoning goes something like this.
> If it is possible for the thing not to be found by normal means, return None.
> If it's only possible if something goes wrong, raise an exception.
I believe the "empty value" as described above is better and I think
that it should be judged from the scope of the function. In this case
there is a function that is supposed to return the version string. When
it fails to do so it should raise an exception or, if it is expected to
find nothing in some cases, the "empty value" of the type it is supposed
to return -- "" in this case -- should be returned. The same applies to
functions that return lists, sets, dicts, etc. That prevents a lot of
bugs where the calling code tries to iterate None, uppercase None, get
key from None, etc.

> This is always a judgement call but here I think not finding version.py
> qualifies as normal and the fact that the version in the original patch returns a faked up
> value kind of supports that. It is calling code that should deal with the possibility
> of None. In this case, that requires one change in anaconda, which is an improvement anyway,
> 
>  op = AnacondaOptionParser(version="%prog " + getAnacondaVersion()
> 
> should change to something involving a string format operation instead of string concatenation.
> It would also require a change in exceptions.initExceptionHandling() but that has to change
> anyway.
> 
> If not finding a version is really an exception-worthy circumstance,
> then raising an exception is the correct thing to do, and I would
> be totally fine with that.
I believe that from the function's point of view, not finding the module
it tries to import is a failure that should be properly "reported" with
as much information as available. It should be left to the caller to
decide whether it is an acceptable failure or not.

> 
> The method in question is not the most crucial method ever, but there seems
> to be a very important principle involved, hence my long response.
Same here and I'm glad this discussion has been started. :)

> If there's something about this issue I'm missing that is perhaps Python
> specific, I'ld like to know.
Described above, I hope.

For your additional note -- I believe the function shouldn't care about
the caller. The function defines its API which the caller should adapt
to. Of course the function should be designed in the way it is expected
to be used, but it should clearly document its behaviour if it is
something potentially problematic (which returning a value of a
different type than it is expected to return is, I think).

-- 
Vratislav Podzimek

Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic




More information about the anaconda-patches mailing list