| msg47690 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-02-06 17:39 |
| cookielib.LWPCookieJar and .MozillaCookieJar are documented to raise cookielib.LoadError on attempt to load an invalid cookies file, but do not. I think this should be backported to the 2.4 maintenance branch. Reason: I suspect more people will be bitten by the bug than will be bitten by the patch, since cookies files will rarely be invalid, so people are likely to have written except statements based on the docs in this case, rather than based on the actual exception that currently occurs. |
|
|
| msg47691 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2005-02-08 16:58 |
| Logged In: YES user_id=764593 I often look at the code in a second idle window rather than starting a web browser. Would it work to make LoadError a subclass of IOError, at least for the backport? People who followed the docs will get a bugfix, but people who followed the code would get no breakage. Should LoadError be a subclass of IOError even in the main trunk? |
|
|
| msg47692 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-02-10 22:25 |
| Logged In: YES user_id=261020 Jim, I don't understand the first sentence in your comment. Re a 2.4 backport that makes LoadError derive from IOError: it makes me wince, but I can't think of an argument against it. No, LoadError should not be a subclass of IOError in the trunk, because the cases where LoadError is documented to be raised do not involve failures of I/O, but rather invalid data. See the docs for IOError. (FWIW, EnvironmentError (IOError's base class) wouldn't be a suitable subclass either: eg. what would we want with an .errno attribute?) |
|
|
| msg47693 - (view) |
Author: Jim Jewett (jimjjewett) |
Date: 2005-02-11 16:06 |
| Logged In: YES user_id=764593 The first sentence was pointing out that many people (including me) never see the pretty-for-printing documentation, and even the html form isn't generally convenient when I'm actually programming. So I rely on the introspection tools. I see object names, signatures, and docstrings. If I need more information, I look at the code, which raises IOError. While I don't yet have code catching this particular error, I do write in a way that would break; it wouldn't have occurred to me to put both types of error in an except clause just in case it changed later. (Well, unless the docstring or at least a comment in the code itself warned me to.) So I would definately prefer that it remain (at least a subclass of) IOError for at least 2.4.x I would also appreciate comments in the fixed 2.4 code if it is going to change for 2.5. |
|
|
| msg47694 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-02-11 18:57 |
| Logged In: YES user_id=261020 I agree the backport (but not the trunk) should have LoadError derive from IOError. I'd be happy for comments to go in noting the change in all places where LoadError is raised, if that's considered good practice. Any committers reading: should I submit an updated patch with just those added comments? Should I submit a patch for the backport, with just that change plus the added (IOError) for backwards-compat.? (Jim: Besides the point for the matter at hand, but I should point out that the "pretty-for-printing" cookielib docs and the docstrings in cookielib.py form almost disjoint sets. Most of the documentation lives under Doc/lib, the stuff in the module source is the more obscure detail. And of course, you have just demonstrated to yourself how not reading the docs leaves you with an incomplete understanding of the intent, which can be useful! Not that I *ever* do that, of course ;-) |
|
|
| msg47695 - (view) |
Author: Martin v. Löwis (loewis) *  |
Date: 2005-03-03 11:03 |
| Logged In: YES user_id=21627 Is it true that LoadError is currently not used anywhere? If so, I think it would be best to eliminate this exception, both from the code and the documentation. To support code that might refer to LoadError already, you could make it a synonym to IOError. |
|
|
| msg47696 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-03-04 14:16 |
| Logged In: YES user_id=261020 LoadError is not currently not used anywhere. Without LoadError, how would one distinguish (for the purpose of error handling) an error due to, on the one hand, a missing file, insufficient permissions, etc. (IOError) from an error due, on the other hand, to an invalid cookies file (LoadError)? This is already a problem with IOError, of course, but I don't see why we should make it worse. |
|
|
| msg47697 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2005-03-04 14:27 |
| Logged In: YES user_id=21627 Can you give an example of an invalid cookies file? How does the library respond to it right now? |
|
|
| msg47698 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-03-04 14:40 |
| Logged In: YES user_id=261020 >>> open('bad_cookies.txt', 'w').write("blah") >>> import cookielib >>> mcj = cookielib.MozillaCookieJar(filename="bad_cookies.txt") >>> mcj.load() Traceback (most recent call last): File "", line 1, in ? File "c:\Python24\lib\cookielib.py", line 1727, in load self._really_load(f, filename, ignore_discard, ignore_expires) File "c:\Python24\lib\_MozillaCookieJar.py", line 53, in _really_load raise IOError( IOError: bad_cookies.txt does not look like a Netscape format cookies file >>> |
|
|
| msg47699 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2005-03-04 14:52 |
| Logged In: YES user_id=21627 I see. In retrospect, it might have been best to reject the cookielib for Python 2.4, and wait for it to get a stable interface and implementation. Without the time machine, we have to accept the consequences, though, so we cannot break existing code. Therefore, I now propose that LoadError becomes a subclass a *permanent* subclass of IOError, thus preserving the historical interface. |
|
|
| msg47700 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-03-04 18:39 |
| Logged In: YES user_id=261020 Re IOError: OK, I'll submit a revised patch. Yes, I agree on your first para, with hindsight. I attempted to avoid making users change interfaces twice (once for a release of a Python 2.4 candidate, once for 2.4 itself). Stupid idea, especially since the changes (though "minor") touched a lot of code. Even then, I wonder if it would have received any significant testing in practice from people other than me, since people might not have bothered to switch from the backwards-compatible (<=2.4) version. Thanks for your attention to these bugs. |
|
|
| msg47701 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-03-04 23:56 |
| Logged In: YES user_id=261020 Revised patch attached. |
|
|
| msg47702 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-03-05 15:28 |
| Logged In: YES user_id=261020 This comment is not relevant to the patch, just a comment on my own comment: > Even then, I wonder if it would have received any significant testing in practice from people other than me, since people might not have bothered to switch from the backwards-compatible (<=2.4) version. Hmm, that's not true: four bugs might have been picked up if I'd released a final version of the <=2.4 backwards-compatible ClientCookie 0.9 version (with the interface changes from ClientCookie 0.4 to cookielib), then released a ClientCookie 1.0 after Python 2.4 came out (there was enough time without needing to wait for 2.5). That's what I should have done, instead of trying to protect ClientCookie users from two rounds of interface changes. Live and learn, I suppose. OTOH, no guilt on 1117339 or 1028908 (except regret that I was suddenly unable to work on it for a period up to the 2.4 beta release, hence missing deadline to get the latter applied before it was released). |
|
|
| msg47703 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-12-04 20:20 |
| Logged In: YES user_id=261020 Patch loaderror_v2.patch implements MvL's suggestion (see followup 2005-03-04 14:52), and includes tests. It still applies cleanly (except that 'patch' doesn't like the ',v' on the end of the CVS patch filenames) and the tests pass. Can it be committed, and preferably backported to the 2.4 maintenance branch? |
|
|
| msg47704 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-12-04 23:09 |
| Logged In: YES user_id=261020 jjlee> It still applies cleanly (except that 'patch' doesn't like the ',v' on the end of the CVS patch filenames) Sorry, FWLIW, it *does* apply cleanly, I just had the wrong -p argument to patch. |
|
|
| msg47705 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-12-05 22:54 |
| Logged In: YES user_id=261020 OK, another patch applied since March causes loaderror_v2.patch to add a class statement which is already present in the patched file. loaderror_v3.patch fixes that. |
|
|
| msg47706 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2005-12-23 21:32 |
| Logged In: YES user_id=33168 Should there be doc changes for LoadError subclassing IOError? Should the re-raise of IOError be changed to raise LoadError? Committed revision 41798. Committed revision 41799. (Misc/NEWS) Committed revision 41800. (2.4) |
|
|
| msg47707 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-12-28 23:04 |
| Logged In: YES user_id=261020 Thanks Neil (and thanks for applying my other cookielib patch). I've attached a doc patch. I don't like to repeat the wording (as the patch does), but it seems necessary here -- do you agree? The re-raise should not be changed, since CookieJar.load() may legitimately raise exactly IOError in addition to LoadError (and was always documented as such). For example, the IOError should be re-raised when the cookies file does not exist. |
|
|
| msg47708 - (view) |
Author: John J Lee (jjlee) |
Date: 2005-12-28 23:17 |
| Logged In: YES user_id=261020 jjlee> and was always documented as such To be precise, it was documented under .revert(). I attach an improved doc patch (loaderror_docs_v2.patch, obsoleting loaderror_docs.patch) that moves that into the .load() method docs, and makes the .revert() docs refer to that. |
|
|
| msg47709 - (view) |
Author: Neal Norwitz (nnorwitz) * |
Date: 2006-01-03 02:14 |
| Logged In: YES user_id=33168 I updated the docs from the v2 patch. It doesn't seem to need backporting. I didn't love the duplication, but it's not a big deal, so I kept it. I modified the wording some to be a bit. Committed revision 41887. |
|
|