Changes between and of Initial VersionVersion 1Ticket #4847, comment 32


Ignore:
Timestamp:
12/15/2014 12:38:26 AM (7 years ago)
Author:
acapnotic
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #4847, comment 32

    initial v1  
    1 As for the test_endpoints.SerialPortEndpointsTestCase.test_connectFailure failure, it seems like the intent of this is to make sure the endpoint.connect() call does return a deferred in the failure case. I think the test specifies SerialException as a response to an earlier review's comment about "You should test a more specific exception than Exception," and SerialException is indeed more specific. However, IStreamClientEndpoint is documented as returning [http://twistedmatrix.com/documents/current/api/twisted.internet.error.ConnectError.html ConectError] on failure, so it should probably be one of those. I am not sure what Twisted best practice is on exception chaining is. twisted.internet.error.getConnectError may be appropriate here, as SerialError seems to conform to the (errno, message) argument signature that getConnectError expects.
     1As for the test_endpoints.SerialPortEndpointsTestCase.test_connectFailure failure, it seems like the intent of this is to make sure the endpoint.connect() call does return a deferred in the failure case. I think the test specifies SerialException as a response to an earlier review's comment about "You should test a more specific exception than Exception," and SerialException is indeed more specific. However, IStreamClientEndpoint is documented as returning [http://twistedmatrix.com/documents/current/api/twisted.internet.error.ConnectError.html ConnectError] on failure, so it should probably be one of those. I am not sure what Twisted best practice is on exception chaining is. twisted.internet.error.getConnectError may be appropriate here, as SerialError seems to conform to the (errno, message) argument signature that getConnectError expects.
    22
    33I do think that should be "except Exception" instead of bare "except:", so as to not catch SystemExit, although a lot of the other code in endpoints seems to do this same thing.