From: Magnus L. H. <ma...@he...> - 2004-07-27 16:07:20
|
Just a general comment on the use of isinstance -- I think it's best avoided when possible. Type checking breaks polymorphism... Signature checking or the "leap before you look" paradigm are, IMO, more Pythonic. Just noticed a few isintance calls in the code here and there -- just thought I'd pipe in. Feel free to ignore :) -- Magnus Lie Hetland "Canned Bread: The greatest thing since sliced http://hetland.org bread!" [from a can in Spongebob Squarepants] |
From: Joerg L. <jo...@us...> - 2004-07-27 16:25:37
|
Hi Magnus, On 27.07.04, Magnus Lie Hetland wrote: > Just a general comment on the use of isinstance -- I think it's best > avoided when possible. Type checking breaks polymorphism... Signature > checking or the "leap before you look" paradigm are, IMO, more > Pythonic. Just noticed a few isintance calls in the code here and > there -- just thought I'd pipe in. Feel free to ignore :) In general, you're right - and there are probably some places where avoiding isinstance would be better. However, there are some places where this is not so easy, and a simple isinstance is clearer than code which tries to generalize even the special case. Sometimes we also check for instances of a specific class to throw an exception already quite early. In principle this is not necessary, but believe me, it helps when you now early on that you did something wrong... Another typical use of isinstance is to implement some type based polymorphism. For instance, if you pass some object to a function and this function checks in which way he can deal with this object. Here, I have to admit, I'm not always sure about the Python way of doing things like that... A typical example was the constructor of the unit.length class (only in the released PyX versions), which was able to convert nearly anything to a length. In CVS we already got rid of that... Jörg |
From: Magnus L. H. <ma...@he...> - 2004-07-27 16:46:58
|
Joerg Lehmann <jo...@us...>: > [snip] > Another typical use of isinstance is to implement some type based > polymorphism. For instance, if you pass some object to a function and > this function checks in which way he can deal with this object. Sure -- but in Python the Right Thing(tm) in most cases isn't type-base polymorphism, but signature-based polymorphism. If something quacks like a duck, you treat it like a duck. It doesn't really have to *be* a duck. (Someone might have implemented a duck-robot, for example, and it would be impolite not to let them use that instead :) > Here, I have to admit, I'm not always sure about the Python way of > doing things like that... A typical example was the constructor of > the unit.length class (only in the released PyX versions), which was > able to convert nearly anything to a length. In CVS we already got > rid of that... OK, let's take a look at that as an example... First of all, it uses helper.isstring and helper.isnumber, both of which use the Leap Before You Look idiom -- I have no quarrels with them. That is not type checking/type based polymorphism. This is how it *should* be done <wink> The only use of isintance here is: if isinstance(l, length): self.length =3D l.length elif helper.isstring(l): # ... Why is this check necessary? Why not use something like this (possibly wrapped in helper.islength or something, if desired): try: self.length =3D l.length except AttributeError: if helper.isstring(l): # ... Then, if I decided to implement my own length-like class which supplied the required length attribute (which is, after all, all that's required) it would work nicely. The only thing you achieve by using isintance instead is to close the door in my face -- "no wannabe lengths permitted". Such type-based discrimination <wink> is in most cases necessary; if an object can do the job, just let it. (Often, using polymorphism directly, i.e. simply calling a method on the given object, would be better than using an if statement checking properties on the object. That's not feasible in special cases such as this, where you have to deal with numbers and strings and the like, of course.) > J=F6rg --=20 Magnus Lie Hetland "Canned Bread: The greatest thing since sliced http://hetland.org bread!" [from a can in Spongebob Squarepants] |
From: Joerg L. <jo...@us...> - 2004-07-27 17:02:34
|
On 27.07.04, Magnus Lie Hetland wrote: [snip] > OK, let's take a look at that as an example... Ok, but as I said, the code in CVS is completely different. Maybe I should have picked another example... > First of all, it uses helper.isstring and helper.isnumber, both of > which use the Leap Before You Look idiom -- I have no quarrels with > them. That is not type checking/type based polymorphism. This is how > it *should* be done <wink> > > The only use of isintance here is: > > if isinstance(l, length): > self.length = l.length > elif helper.isstring(l): > # ... > > Why is this check necessary? Why not use something like this (possibly > wrapped in helper.islength or something, if desired): > > try: > self.length = l.length > except AttributeError: > if helper.isstring(l): > # ... > > Then, if I decided to implement my own length-like class which > supplied the required length attribute (which is, after all, all > that's required) it would work nicely. > > The only thing you achieve by using isintance instead is to close the > door in my face -- "no wannabe lengths permitted". You're right, this should not be the case. Sometimes, however, we introduced some checks such that the user doesn't get hit by some strange error when he calls writeEPSfile. For instance, we check whether the arguments passed to some canvas methods are of a certain type. I don't think this is necessarily bad. See for instance the insert and set method of the canvas._canvas class for two examples. > Such type-based discrimination <wink> is in most cases necessary; if > an object can do the job, just let it. > > (Often, using polymorphism directly, i.e. simply calling a method on > the given object, would be better than using an if statement checking > properties on the object. That's not feasible in special cases such as > this, where you have to deal with numbers and strings and the like, of > course.) Again, you're right - which unfortunately does not mean that the code is perfect in this sense - it developped over quite some time... So if there are some points where you think the present code can be improved in this regard, feel free to point them out (or just send a patch). Jörg |
From: Magnus L. H. <ma...@he...> - 2004-07-27 17:42:09
|
Joerg Lehmann <jo...@us...>: > [snip] > You're right, this should not be the case. Sometimes, however, we > introduced some checks such that the user doesn't get hit by some > strange error when he calls writeEPSfile. Checks are good. I guess I'm just saying that more flexible (i.e. signature-based) checks could be used instead. > For instance, we check whether the arguments passed to some canvas > methods are of a certain type. I don't think this is necessarily > bad. See for instance the insert and set method of the > canvas._canvas class for two examples. The best solution here, IMO, (in an ideal world ;) would be to use either (1) interfaces or (2) protocol adaptation. There are PEPs for both, but I'm not sure whether either will ever become standard Python. (I guess interfaces have the best chance, even though adaptation is much cooler ;) If the API is simple one could check manually (as with the helper functions for strings etc.; you could write similar ones to check whether something seems to be a good canvas). I guess in these cases a simple isinstance might be easier. After all, we *do* have multiple inheritance, so one could always just subclass the needed class as if it were an interface. > > Such type-based discrimination <wink> is in most cases necessary; if > > an object can do the job, just let it. > > > > (Often, using polymorphism directly, i.e. simply calling a method on > > the given object, would be better than using an if statement checking > > properties on the object. That's not feasible in special cases such a= s > > this, where you have to deal with numbers and strings and the like, o= f > > course.) >=20 > Again, you're right - which unfortunately does not mean that the code i= s > perfect in this sense - it developped over quite some time... So if > there are some points where you think the present code can be improved > in this regard, feel free to point them out (or just send a patch). The reason I noticed this was that I saw plenty of isinstance() calls in the linear equation code -- might be inconvenient if one wanted to mix in multidimensional points between the variables and numbers it already knows about, because, basically, the "aren't allowed" in there. No big deal, though. If it turns out to be problematic, I can always fiddle a bit with the code, I guess :) > J=F6rg --=20 Magnus Lie Hetland "Canned Bread: The greatest thing since sliced http://hetland.org bread!" [from a can in Spongebob Squarepants] |
From: Andre W. <wo...@us...> - 2004-07-28 06:09:52
|
Hi, On 27.07.04, Magnus Lie Hetland wrote: > I guess in these cases a simple isinstance might be easier. After all, > we *do* have multiple inheritance, so one could always just subclass > the needed class as if it were an interface. First of all, I agree to you, that we should use try-except clauses instead of isinstance almost everywhere. But we do (maybe missuse) the class structure and isinstance in one place, where I think its worth to be discussed in order to make clear, whether its a design mistake from the very beginning. I'm not so sure. Let me explain it a little. It's related to the attribute system. Suppose you call the fill method or the draw method of a canvas and provide some additional attributes like colors, linewidths, linestyles, patterns, etc. (Additionally, the fill and draw methods can take some decorators, which might add other stuff (like arrows) and shorten the path (for the head of the arrow and the like). BTW Jörg, Michael: we wanted to restrict ourselfs to not really modify the original path anymore by decorators, which are now called deformers. Did anybody the coding for that? The decorated path should be simplified in that sense. We need to come back to that issue at some point. Maybe in Basel, Jörg? But back to the draw and fill attributes.) So what we did is, we introduced a strokestyle and fillstyle class, which is used as a mix-in to certain attributes. The idea is, that a color can be used as a stroke as well as a fill style, while a line width is usefull when stroking only. Once we mix-in the strokestyle and/or fillstyle class, we can use isinstance to check, whether the style is appropriate to be used at a given point. (Isn't that's what Jim Fulton always says, that __implements__ is something else that a position in a class hierarchy.) I know, that we created some restriction here: you can't derive a class from, say, the line width, which will not be a stroke attribute anymore. But I don't care (we really don't need to take care). I think, at this very point, the isinstance is the most simple check possible. We could define some "ask-what-it-is" functionality on attributes as a whole, but I'm not sure we really need this. (BTW: it is neither very complicated to change this at a later point, nor would it be visible to the outside world. So I can live this this issue quite well as it currently is -- independing from what you're telling me ;-).) (As a side remark: The merging an clearing-functionality of the attributes is based on isinstance as well ...) > The reason I noticed this was that I saw plenty of isinstance() calls > in the linear equation code -- might be inconvenient if one wanted to > mix in multidimensional points between the variables and numbers it > already knows about, because, basically, the "aren't allowed" in > there. Right. Don't take the isinstance code too serios right now. I added quite some isinstance checks and assert checks to help me getting it running properly. André -- by _ _ _ Dr. André Wobst / \ \ / ) wo...@us..., http://www.wobsta.de/ / _ \ \/\/ / PyX - High quality PostScript figures with Python & TeX (_/ \_)_/\_/ visit http://pyx.sourceforge.net/ |
From: Magnus L. H. <ma...@he...> - 2004-07-28 19:54:02
|
Andre Wobst <wo...@us...>: > > (BTW: it is neither very complicated to change this at a later > point, nor would it be visible to the outside world. So I can live > this this issue quite well as it currently is -- independing from > what you're telling me ;-).) That's fine -- as long as you know what you're doing (and you do, of course, know your code much better than me). :) I've just had a deep-rooted fear and loathing for type checking instilled in me by Alex Martelli ;) [snip] > Right. Don't take the isinstance code too serios right now. I added > quite some isinstance checks and assert checks to help me getting it > running properly. Quite all right. > Andr=E9 --=20 Magnus Lie Hetland "Canned Bread: The greatest thing since sliced http://hetland.org bread!" [from a can in Spongebob Squarepants] |