| msg41667 - (view) |
Author: John J Lee (jjlee) |
Date: 2002-11-15 22:25 |
| Remove undesirable type-checking assertion from urllib2.Request. |
|
|
| msg41668 - (view) |
Author: Neal Norwitz (nnorwitz) *  |
Date: 2002-11-15 23:14 |
| Logged In: YES user_id=33168 John, could you explain why you need it and what is the benefit? |
|
|
| msg41669 - (view) |
Author: John J Lee (jjlee) |
Date: 2002-11-17 14:46 |
| Logged In: YES user_id=261020 It's widely regarded as a bug if Python code checks for type with isinstance (or type(foo) == type(bar)) without some good reason. It's plausible that you may want to make an object that implements the Request interface without deriving from Request (say, I don't know, to implement the frobozz URI scheme, which requires ordered headers, and never has any data associated with it). If so, you don't want to have to follow 'bug fixes' in the Python std. library that may break your code simply because you had to derive from Request to satisfy the assertion. I might have done this when I wrote a couple of modules that build on urllib2, actually. I'm not sure whether that would have been the best way, because I didn't think about it since I didn't have any choice in the matter, thanks to this assertion! OTOH, it's true that removing type-checks can break backwards compatibility. However, this is an assertion, not a real runtime type-check, so it won't break backwards compatibility: if people are relying on catching AssertionError to do type-checking in their own code, that's their problem! The docs say: urlopen(url[, data]) Open the URL url, which can be either a string or a Request object (currently the code checks that it really is a Request instance, or an instance of a subclass of Request). Note the 'currently' (and the source code comment indicating that what we really want to check is the interface), and that fact that the code *doesn't* actually check it, but only asserts. Request interface is already documented, so there's no problem there. John |
|
|
| msg41670 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2002-11-17 17:55 |
| Logged In: YES user_id=80475 I see no problem with weakening the assertion, but hasattr should check for a required part of the interface instead of a new, undocumented, dummy attribute. |
|
|
| msg41671 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2002-11-17 17:57 |
| Logged In: YES user_id=80475 I see no problem with weakening the assertion, but hasattr should check for a required part of the interface instead of a new, undocumented, dummy attribute. |
|
|
| msg41672 - (view) |
Author: John J Lee (jjlee) |
Date: 2002-11-17 21:21 |
| Logged In: YES user_id=261020 Why not a new attribute? What would it break? Checking for the interface by checking all the methods (there are maybe ten of them) is not really practical, and really it's the intent that's the important bit. |
|
|
| msg41673 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-09 01:32 |
| Logged In: YES user_id=357491 It's not so much that the dummy attribute would break anything so much as it is just unneeded. If you want to make sure an object is not going to meet your required API you either just follow EAFP (Easier to Ask Forgiveness than Permission) or you explicitly test for the required interface. There is no good argument to have to flag an object to say that it meets an API spec. |
|
|
| msg41674 - (view) |
Author: John J Lee (jjlee) |
Date: 2003-05-09 12:43 |
| Logged In: YES user_id=261020 Well, I think EAFP is probably best. An attribute check is in some sense the nearest thing to the assert that's there now, which is why my patch did that. OTOH, a method check seemed half-baked to me and so worse than an attribute check. it's not worth *too* much of our time :-) |
|
|
| msg41675 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-09 23:47 |
| Logged In: YES user_id=357491 =) OK. If you care to rewrite the patch using EAFP, then please do so. If not, then you just followup here saying you don't think the patch is worthing persuing any farther so I can close it? |
|
|
| msg41676 - (view) |
Author: John J Lee (jjlee) |
Date: 2003-05-10 11:58 |
| Logged In: YES user_id=261020 Is there a 'least interesting patch of the year' prize? My entry is attached. |
|
|
| msg41677 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-11 05:33 |
| Logged In: YES user_id=357491 This is not what I meant by EAFP. In order for a patch to do that you would need to wrap all outgoing calls in that method that will use 'req' and see if the exception was because the object didn't have the proper interface. Good try, though. =) Doing it using hasattr in a loop is probably going to be the best bet. If you don't want to do the patch I will understand; I should have been more clear by what I was expecting for EAFP. Thus if you don't want to do it I will do the patch myself. |
|
|
| msg41678 - (view) |
Author: John J Lee (jjlee) |
Date: 2003-05-11 11:46 |
| Logged In: YES user_id=261020 No!! Ahem. Personally I wouldn't want to apply a patch like that -- I think it's inappropriate, since this is a violated precondition, and the patch you describe would add significant complexity (not to mention inefficiency, if I dare use that word here). The caller will get AttributeError anyway, with my patch, and though it wasn't my goal to pamper callers who mess up the call, that's actually not an unreasonable error. Would it convince you if I pointed out that my patch is simply EAFP again, at a different place in the call stack? If you don't like AttributeError being raised, you could always wrap up the five uses of req in try: except: blocks, but personally I don't think that's worth the added complexity or bug-masking risk. I don't know... first he tells me not to attribute check, then he wants to do it in a loop ;-) |
|
|
| msg41679 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-11 23:24 |
| Logged In: YES user_id=357491 I am going to see if i can get someone to step in here and give me some advice on how to handle this. I personally have no issues ditching the check entirely. I just checked the module and it does not look like an AttributeError will be caught somewhere else in the code. But since Raymond only wanted to weaken the assertion and not remove it I feel the need to get someone else to choose a side on this to force which resolution is taken (status quo, hasattr, remove the check entirely). |
|
|
| msg41680 - (view) |
Author: John J Lee (jjlee) |
Date: 2003-05-12 00:59 |
| Logged In: YES user_id=261020 Oh, is it really worth any more of everyone's time, I wonder? I didn't anticipate a big discussion. Let's just leave it at the status-quo, shell we? |
|
|
| msg41681 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-12 06:55 |
| Logged In: YES user_id=357491 Too late, I got approval. =) I have to anyway, John, since this is one of the first patches I have handled personally. The assert will be removed some time this week. |
|
|
| msg41682 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2003-05-12 07:32 |
| Logged In: YES user_id=357491 Applied as Lib/urllib2.py 1.44 |
|
|