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

Anne Mulhern amulhern at redhat.com
Thu May 22 12:48:49 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> Sent: Wednesday, May 21, 2014 3:47:38 AM
> Subject: Re: [PATCH] Move Anaconda version detection from isys to Python code
> 
> 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.
> 

It hadn't occurred to me before, because I've only seen these types in
statically checked languages, where their importance is to the type-checker
more than anything else, but actually a Maybe type would, I think,
be useful in a dynamically checked language like Python. All accesses
to an object would fail until the possibility of a None value was
accounted for, which seems like it would be better than waiting
until the None value eventually crops up. But a Maybe type
would still be a type that exists to be a type rather than because
it provides any actual functionality, which would be kind of a big step
for Python.

In any case, it doesn't really help with the current problem.

> > 
> > 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.
> 

This is true of every type error in a dynamically checked language and
Lisp programmers have their own version of that hell. I think you're
saying that "NoneType has no attribute..." errors are a substantial
portion of the total type errors that Python programmers generally
see and I have no doubt that you are correct.

> 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.

Yes, that's the case so long as the exceptions are chosen carefully.

> 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.
> 

I would return an exception where you would return an exception, but
None where you might return, say "". They pack the same amount of
information about the reason for not getting the value, so we don't
really have much to argue about there.

> > 
> > 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.
> 

Well a real life example of where this can go slightly wrong which I encounter
arises from my habit of not filling in my first name wherever I can get away
with it. This results in the occasional form letter or thing like that
with a salutation like:

Dear ,

<body of letter>

That empty string prevented the code from crashing, but it's now a silent logical
bug. Again, it's no big deal, because I'm not going to read the letter anyway, still
it's an example. Or, we imagine some sort of algorithm that times out after a certain
number of iterations, at which point it may not have found the answer.
This is part of its natural working, but to return, say, the empty set, which
could be an actual answer would be very wrong, it should return None.
Situations like these really arise as I'm sure you know.

That's the problem of an empty value being overloaded to mean two things in
circumstances where the calling program needs to distinguish between the two
things and I think it happens more than people really realize.

The other problem is that there are many classes where this empty value doesn't
exist, and we have to deal with those as well, both as the return values of
function calls and as fields in objects. This forces you to always have a
mixed approach and can lead to certain headaches in refactoring as well
if you want to upgrade from one of these builtins that does have an empty
value to some other class.

> > 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).
> 

Well, in my algorithm example, these ideas work together. None is a legitimate
return value and the calling code will naturally invoke the function
in a way that expects and handles None.

It feels like our disagreement is really more about precisely how to
judge individual situations than about anything fundamental. We all
know that Haskell has monads for this little problem and programs
written in any other language just stagger along as best they can.

About the little function that started it all, getAnacondaVersion()
...if I view it as a convenience wrapper around the actual function (which 
correctly returns None but has been inlined into getAnacondaVersion()
because it seems too trivial to be kept separate at least at this time)
...then I'm happy with it returning a non-empty string in all cases.
I think it should return "unknown", not "unknown version", though.
To be super proper it should be renamed something like
displayableAnacondaVersion(), or something like that.

- mulhern

> --
> 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
> 


More information about the anaconda-patches mailing list