Patch #1752270 seems more idiomatic (TypeError, not ValueError) and mildly less intrusive (it does not change the return value of urllib2.OpenerDirector.add_handler()). Also, your patch adds a backwards incompatibility which the new one does not: Currently a correct call to urllib2.build_opener() may return successfully without adding all of its handler arguments to the returned urllib2.OpenerDirector, since a urllib2.BaseHandler instance may have no appropriate methods (no .http_open(), etc. etc.). Your patch causes build_opener() (but not .add_handler()) to raise ValueError in that case. One could argue that it should raise an exception, but there is still scope for breakage, e.g. since these attributes may be added at runtime, and the number of added attributes may happen to be zero (and in fact, urllib2.ProxyHandler falls into this category).
I think the docs do not need to be updated if my revised patch is applied, since they already state that urllib2.OpenerDirector.add_handler() accepts only urllib2.BaseHandler instances (and typically the possibility of a TypeError being raised is not explicitly documented in stdlib docs).
|