msg25090 - (view) |
Author: Matt Roper (mattrope) |
Date: 2005-04-20 19:52 |
Python's nntplib currently raises a generic EOFError if
the connection is closed unexpectedly. This seems
inconsistent with other Python libraries (smtplib,
imaplib, etc.) and is unexpected behaviour. It seems
that a new Exception class derived from the NNTPError
(e.g., NNTPConnectionError) should be used instead.
As it stands now, the only indication that EOFError can
be raised is in the docstring for the internal
getline() method. There is no mention in the
documentation that higher level methods call getline()
and can raise the EOFError to the application level.
If no new exception class is added for this situation,
it would be nice to have this behaviour noted in the
documentation.
|
msg114494 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2010-08-21 13:55 |
The OP would accept a documentation change if the code's not going to be changed.
|
msg222685 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-07-10 18:24 |
I am going to fix it so that it raises the NNTPConnectionError rather than update the documentation.
|
msg222711 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-07-10 23:04 |
I have a fix and added some test coverage in order to make sure the NNTFConnectError was being called. However, in the test case, I am monkey patching. If there is a way to do this with mock, I would appreciate the feedback.
|
msg222958 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2014-07-13 17:56 |
diff -r 8f85262fbe8a Lib/nntplib.py
--- a/Lib/nntplib.py Sun Jul 06 02:24:24 2014 -0400
+++ b/Lib/nntplib.py Thu Jul 10 16:10:38 2014 -0700
@@ -122,6 +122,9 @@
"""Error in response data"""
pass
+class NNTPConnectError(NNTPError):
+ """Error during connection establishment."""
+ pass
Could you also document the new exception? (See Doc/library/nntplib.rst and please add a versionadded directive)
The pass statement is redundant, but we could keep it to be consistent with rest of the library.
@@ -435,7 +438,7 @@
raise NNTPDataError('line too long')
if self.debugging > 1:
print('*get*', repr(line))
- if not line: raise EOFError
+ if not line: raise NNTPConnectError()
if not line:
raise NNTPConnectError
looks more readable to me.
Also, you could add a more descriptive error message.
|
msg222960 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2014-07-13 18:04 |
Wouldn't this be backward incompatible?
Even if the EOFError that is raised is not documented explicitly, people might be catching it, and switching to a new exception would break their programs. Maybe NNTPConnectError should inherit from EOFError too?
|
msg222961 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-07-13 18:07 |
That's a good point. I can add that so the NNTPConnectError can inherit the EOFError
|
msg222963 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-07-13 18:39 |
Here is an updated patch.
|
msg224670 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2014-08-04 01:50 |
I could be wrong, but isn’t this error raised when expecting a response from any command, not just during “connection establishment”? Perhaps change the docstring to say something like “Connection closed unexpectedly” instead.
|
msg224725 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2014-08-04 14:18 |
Indeed, I find the description rather vague and potentially misleading. "Connection establishment" would usually refer to the TCP connection, but an EOFError is actually a high-level, logical error (it has nothing to do with networking per se: it's probably more of a server failure - or a client bug perhaps :-)).
|
msg224794 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-08-05 02:11 |
I'ved changed the comment to say Connection closed unexpectedly.
|
msg225634 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2014-08-22 01:47 |
NNTPConnectError does still seem a slightly awkward name. I would go for NNTPConnectionError instead, but I’m also happy to put my bikeshed paint away let this patch be applied as is :)
Handling of NNTPTemporaryError with a code of 400 is similar to handling of this EOFError. But I guess there is not much you could do with the API unless you made a separate subclass for 400 errors (like all the new EnvironmentError/OSError subclasses), which would be rather severe. My current workaround looks a bit like this:
try:
[_, info] = nntp.body(...)
except NNTPTemporaryError as err:
[code, *msg] = err.response.split(maxsplit=1)
if code != "400":
raise
except EOFError: # Or NNTPConnect(ion)Error
msg = ()
else:
break # Handle successful response
[msg] = msg or ("Server shut down connection",)
# Handle connection shutdown by server
|
msg225638 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2014-08-22 02:48 |
Some more points:
* I suggest adding something like this to the documentation:
exception nntplib.NNTPConnect[ion]Error
Exception raised when the server unexpectedly shuts down the connection.
* The tests should use BytesIO rather than StringIO. Other than that, I think monkey-patching the file attribute in the tests is fine; that’s probably the way I would do it!
* The new exception should be added to __all__.
|
msg225640 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2014-08-22 03:04 |
PPS: Documentation should probably have the “New in version 3.5” tag as well
|
msg225641 - (view) |
Author: Lita Cho (Lita.Cho) * |
Date: 2014-08-22 03:13 |
Thank yo so much, Martin! I will incorporate these changes and add them soon!
|
msg245612 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-06-22 02:19 |
Quick survey of other communication protocol modules:
ftplib: uses EOFError; not documented
http.client: custom IncompleteRead exception
imaplib: custom IMAP4.abort exception
poplib: custom error_proto exception
smtplib: SMTPServerDisconnected exception, subclassing OSError
telnetlib: EOFError is documented
|
msg415082 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2022-03-13 19:08 |
nntplib is deprecated as per PEP 594, so there won't be further enhancements to it.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:10 | admin | set | github: 41884 |
2022-03-13 19:08:26 | iritkatriel | set | status: open -> closed
nosy:
+ iritkatriel messages:
+ msg415082
resolution: wont fix stage: patch review -> resolved |
2015-06-22 02:19:38 | martin.panter | set | messages:
+ msg245612 |
2014-08-22 03:13:11 | Lita.Cho | set | messages:
+ msg225641 |
2014-08-22 03:04:22 | martin.panter | set | messages:
+ msg225640 |
2014-08-22 02:48:00 | martin.panter | set | messages:
+ msg225638 |
2014-08-22 01:47:42 | martin.panter | set | messages:
+ msg225634 |
2014-08-05 02:11:38 | Lita.Cho | set | files:
+ nntplib_error_v2.patch
messages:
+ msg224794 |
2014-08-04 14:18:18 | pitrou | set | nosy:
+ pitrou messages:
+ msg224725
|
2014-08-04 01:50:38 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg224670
|
2014-07-13 18:39:13 | Lita.Cho | set | files:
+ nntplib_error.patch
messages:
+ msg222963 |
2014-07-13 18:07:14 | Lita.Cho | set | messages:
+ msg222961 |
2014-07-13 18:04:55 | ezio.melotti | set | nosy:
+ ezio.melotti messages:
+ msg222960
|
2014-07-13 17:56:23 | berker.peksag | set | nosy:
+ berker.peksag
messages:
+ msg222958 versions:
+ Python 3.5, - Python 3.2 |
2014-07-13 17:21:00 | jesstess | set | keywords:
+ needs review stage: test needed -> patch review |
2014-07-10 23:04:17 | Lita.Cho | set | files:
+ nntplib_error.patch keywords:
+ patch messages:
+ msg222711
|
2014-07-10 18:24:15 | Lita.Cho | set | nosy:
+ jesstess, Lita.Cho messages:
+ msg222685
|
2014-02-03 18:39:27 | BreamoreBoy | set | nosy:
- BreamoreBoy
|
2013-03-08 04:53:08 | Ankur.Ankan | set | nosy:
+ Ankur.Ankan
|
2010-08-21 13:55:56 | BreamoreBoy | set | nosy:
+ BreamoreBoy
messages:
+ msg114494 versions:
+ Python 3.2, - Python 2.7 |
2009-02-16 01:01:26 | ajaksu2 | set | stage: test needed versions:
+ Python 2.7 |
2005-04-20 19:52:50 | mattrope | create | |