isinstance() considered harmful

[Kragen Sitaker]

Kosh writes:
> However I do see it as useful to make base classes that esentially
> do nothing but instead encapsulate some kind of behavior so that you
> can use isinstance to test if an object implements that kind of
> behavior.  Otherwise how would you test if an object was able to
> work in a listish type way. You don't want to check every method
> needs to indexing, len etc when you work with the object all the
> time however if you put all those methods in a base class with just
> pass for an implementation then you can test for an insinstance of
> that class and find out very quickly if it is some kind of list
> capable object.

Well, if you use it as if it were a list, and it isn't, it will raise
an exception.  Which I assume is what you do if the isinstance() test
fails, right?  The only difference is that the exception will be an
AttributeError: __len__ or something like that, instead of whatever
hopefully more specific exception you raise if your isinstance()
fails, which will hopefully make it easier for the person who wrote
the calling code to fix things.

You can achieve the same thing by replacing 
    length = len(somearg)
with
    try: length = len(somearg)
    except AttributeError: raise TypeError, "somearg must be a sequence"

Implementing all those methods in a base class with "pass" is a really
stupid idea, unless it's semantically correct for that method to do
nothing, because it turns broken code that would raise exceptions into
broken code that silently gives wrong answers.  If someone implements,
say, append() but not extend(), and their object gets passed to a
routine that does both, then that routine will think it has added
stuff with both append() and extend(), but actually only the stuff it
added with append() will get there.  And you'll be debugging (or maybe
trying to explain to a customer why they got the wrong answer) to try
to figure out where those other values went.

> I use this in zope a fair bit since I can easily check if an object
> is catalogaware, persistent etc. Since each of those items actually
> encomapasses a fair number of methods I don't really see any fast
> way to check for all of these existance during the time I have to
> draw the webpage and hand it to a user.

Well, if it's OK for a particular object not to be catalogaware, you
can say:

try: do_something_involving_cataloging(theobject)
except: pass

Generally I hate except: pass, but it expresses what you want in this
case.

> > It's not just overly conservative; it's also
> > overly liberal.  Someone can override methods in
> > the interface with broken methods --- in Python,
> > they can even override them with non-callable
> > objects.  An object's derivation from a particular
> > class doesn't guarantee that it implements all the
> > protocol that class does.  (Still, breaking
> > protocols your base classes implement is almost
> > always a bad idea.)
> 
> Overall I have final say over all code that we use so someone
> breaking how things work in an inherited class doesn't happen.

If that's the case, then someone passing in an argument that isn't the
right type won't happen either.