-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jean-Paul Calderone wrote:

This branch seems to have no ticket number, so I'm commenting here.

Thanks for the comments, I'll create a ticket for it.

twisted/test/test_endpoints.py:

Several test methods of TestEndpoints define nested functions which discard the Deferred returned by a Port's stopListening method. These need to be waiting on that Deferred.

Why do some tests bind to 127.0.0.1 and some to 127.0.0.2?

The client connection attempts to 127.0.0.2 are there to make sure that the connection fails. It seemed logical at the time, but the time was very late.

None of the tests assert that the protocol/port is hooked up to the right address, either on the local or remote side.

I'll add these.

None of the tests exercise client-side binding.

I'm not sure what you mean by this, but there is a test for each of the endpoint classes that tests that the client can connect to a "normally established server" Perhaps I didn't do this very well.

The TCP and UNIX tests are basically identical, except for the endpoint class being used. This is a lot of unnecessary code duplication. Implement the tests with the endpoint class parameterized and then call them all twice, once with TCPEndpoint, once with UNIXEndpoint.

I wanted to get something that actually worked before I tried to factor out duplicate, code. But thanks for the note anyway.

twisted/internet/endpoint.py:

Interface definitions have docstrings - they don't need "pass" (pass is only required to satisfy grammar requirements for /some/ statement, and the docstrings do just fine for that).

Noted.

The docstrings of neither IClientEndpoint.connect nor IServerEndpoint.listen define the error type which the Deferred may fail with. Is it really true that IServerEndpoint.listen will never callback its Deferred? The tests seem to think otherwise.

I made comments about a deferred never callbacking at the end of the day, because I didn't realize that obviously IServerEndpoint.listen will call back with the IListeningPort, I forgot to adjust the docstrings.

Typo in IServerEndpoint.listen's docstring - "incomfing".

Also, these interfaces belong in twisted.internet.interfaces.

I'll move them there.

_EndpointClientFactory.buildProtocol should not be using callLater(0). There is no guarantee that will be the correct time to deliver notification, and it is unnecessarily inefficient. Instead, you probably want something like a ProtocolWrapper from twisted.protocols.policies.

I'll look into ProtocolWrapper.

self.callable(addr) in the same buildProtocol is user-code. It would probably be useful for exceptions it raises to cause the Deferred associated with the factory to errback. Currently, errors it causes will be logged and the Deferred passed back to the application will never fire or errback.

Similar comment for _EndpointServerFactory.buildProtocol's invocation of self.callable().

Oops

TCPEndpoint.listen - try/bareword except? Why? This looks like a perfect place for twisted.internet.defer.execute().

I didn't know about defer.execute(), but thanks, I'll use it.

AddressToEndpoint looks like it should just be a function that returns either a TCPEndpoint or a UNIXEndpoint.

I figured that out as I woke up this morning.

Trailing whitespace *and tabs* throughout.

*grumble* *grumble* Yet-Another-Carbon-Emacs *grumble* Thanks for the input JP. - -David -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQFD0/dLrsrO6aeULcgRAl8WAJ9YSYusZfZ7d+1T4WvyGWYPnIMTBACfXr/V NTCZJGY+Q/1zsbWEqNWrkhg= =EORJ -----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jean-Paul Calderone wrote:

This branch seems to have no ticket number, so I'm commenting here.

Thanks for the comments, I'll create a ticket for it.

twisted/test/test_endpoints.py:

Several test methods of TestEndpoints define nested functions which discard the Deferred returned by a Port's stopListening method. These need to be waiting on that Deferred.

Why do some tests bind to 127.0.0.1 and some to 127.0.0.2?

The client connection attempts to 127.0.0.2 are there to make sure that the connection fails. It seemed logical at the time, but the time was very late.

None of the tests assert that the protocol/port is hooked up to the right address, either on the local or remote side.

I'll add these.

None of the tests exercise client-side binding.

I'm not sure what you mean by this, but there is a test for each of the endpoint classes that tests that the client can connect to a "normally established server" Perhaps I didn't do this very well.

The TCP and UNIX tests are basically identical, except for the endpoint class being used. This is a lot of unnecessary code duplication. Implement the tests with the endpoint class parameterized and then call them all twice, once with TCPEndpoint, once with UNIXEndpoint.

I wanted to get something that actually worked before I tried to factor out duplicate, code. But thanks for the note anyway.

twisted/internet/endpoint.py:

Interface definitions have docstrings - they don't need "pass" (pass is only required to satisfy grammar requirements for /some/ statement, and the docstrings do just fine for that).

Noted.

The docstrings of neither IClientEndpoint.connect nor IServerEndpoint.listen define the error type which the Deferred may fail with. Is it really true that IServerEndpoint.listen will never callback its Deferred? The tests seem to think otherwise.

I made comments about a deferred never callbacking at the end of the day, because I didn't realize that obviously IServerEndpoint.listen will call back with the IListeningPort, I forgot to adjust the docstrings.

Typo in IServerEndpoint.listen's docstring - "incomfing".

Also, these interfaces belong in twisted.internet.interfaces.

I'll move them there.

_EndpointClientFactory.buildProtocol should not be using callLater(0). There is no guarantee that will be the correct time to deliver notification, and it is unnecessarily inefficient. Instead, you probably want something like a ProtocolWrapper from twisted.protocols.policies.

I'll look into ProtocolWrapper.

self.callable(addr) in the same buildProtocol is user-code. It would probably be useful for exceptions it raises to cause the Deferred associated with the factory to errback. Currently, errors it causes will be logged and the Deferred passed back to the application will never fire or errback.

Similar comment for _EndpointServerFactory.buildProtocol's invocation of self.callable().

Oops

TCPEndpoint.listen - try/bareword except? Why? This looks like a perfect place for twisted.internet.defer.execute().

I didn't know about defer.execute(), but thanks, I'll use it.

AddressToEndpoint looks like it should just be a function that returns either a TCPEndpoint or a UNIXEndpoint.

I figured that out as I woke up this morning.

Trailing whitespace *and tabs* throughout.

*grumble* *grumble* Yet-Another-Carbon-Emacs *grumble* Thanks for the input JP. - -David -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQFD0/dLrsrO6aeULcgRAl8WAJ9YSYusZfZ7d+1T4WvyGWYPnIMTBACfXr/V NTCZJGY+Q/1zsbWEqNWrkhg= =EORJ -----END PGP SIGNATURE-----