Remove undesirable type-checking assertion from 
urllib2.Request.

Logged In: YES 
user_id=33168

John, could you explain why you need it and what is the benefit?

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

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.

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.

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.

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.

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. 
 
<shrug> it's not worth *too* much of our time :-) 
 

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?

Logged In: YES 
user_id=261020

Is there a 'least interesting patch of the year' prize? 
My entry is attached. 
 

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.

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

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

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? 
 

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.

Logged In: YES 
user_id=357491

Applied as Lib/urllib2.py 1.44