[Twisted-Python] Question about FileDescriptor.loseConnection() signature found by mypy
In twisted.internet.abstract.FileDescriptor.loseConnection, the loseConnection method is defined like: def loseConnection(self, _connDone=failure.Failure(main.CONNECTION_DONE)): while in twisted.internet._newtls.ConnectionMixin , we have: def loseConnection(self): If I run *tox -e mypy* in trunk, mypy complains with this: src/twisted/internet/tcp.py:204:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/tcp.py:527:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/tcp.py:780:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/tcp.py:789:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/unix.py:243:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/unix.py:431:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] Do the signatures of loseConnection need to match in order for this to be a valid subclass? Is there any way to fix the code so that the mypy error goes away, or do we need to add a comment to turn off the mypy error here? -- Craig
On Jun 15, 2020, at 8:43 PM, Craig Rodrigues
wrote: In twisted.internet.abstract.FileDescriptor.loseConnection, the loseConnection method is defined like:
def loseConnection(self, _connDone=failure.Failure(main.CONNECTION_DONE)):
I think this signature might just be... wrong? ITransport doesn't include it. Does anything actually use this argument? -g
On Tuesday, 16 June 2020 10:51:21 CEST Glyph wrote:
On Jun 15, 2020, at 8:43 PM, Craig Rodrigues
wrote: In twisted.internet.abstract.FileDescriptor.loseConnection, the loseConnection method> is defined like: def loseConnection(self, _connDone=failure.Failure(main.CONNECTION_DONE)): I think this signature might just be... wrong? ITransport doesn't include it. Does anything actually use this argument?
I think this isn't actually an argument: the underscore in the name suggests it is not part of the interface. What seems to be happening here is that twisted.internet.abstract.FileDescriptor.loseConnection defines a constant by putting it in an argument default value. A simple solution would be to replace it by a class-scope or module- scope constant. By the way, is it valid to wrap an exception that was never raised in a Failure object? Because that is what happens here by reusing the single _connDone instance. It seems Failure's implementation is prepared to handle it, but its docstring doesn't mention it. Bye, Maarten
twisted.internet.abstract.FileDescriptor.loseConnection() was changed here
in 2004:
https://github.com/twisted/twisted/commit/081d393ab03da92d744d8fb2b5d7770566...
This change was done so long ago, but I think changing the signature like
that was wrong.
I took a guess at how to fix this and submitted this:
https://github.com/twisted/twisted/pull/1292
--
Craig
On Tue, Jun 16, 2020 at 1:52 AM Glyph
On Jun 15, 2020, at 8:43 PM, Craig Rodrigues
wrote: In twisted.internet.abstract.FileDescriptor.loseConnection, the loseConnection method is defined like:
def loseConnection(self, _connDone=failure.Failure(main.CONNECTION_DONE)):
I think this signature might just be... wrong? ITransport doesn't include it. Does anything actually use this argument?
-g
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On 6/15/20 11:43 PM, Craig Rodrigues wrote:
In twisted.internet.abstract.FileDescriptor.loseConnection, the loseConnection method is defined like:
def loseConnection(self, _connDone=failure.Failure(main.CONNECTION_DONE)):
while in twisted.internet._newtls.ConnectionMixin , we have:
def loseConnection(self):
If I run *tox -e mypy* in trunk, mypy complains with this:
src/twisted/internet/tcp.py:204:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc]
src/twisted/internet/tcp.py:527:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/tcp.py:780:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/tcp.py:789:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc]
src/twisted/internet/unix.py:243:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] src/twisted/internet/unix.py:431:1: error: Definition of "loseConnection" in base class "ConnectionMixin" is incompatible with definition in base class "FileDescriptor" [misc] Do the signatures of loseConnection need to match in order for this to be a valid subclass? Is there any way to fix the code so that the mypy error goes away, or do we need to add a comment to turn off the mypy error here?
Not directly related, but I ran into a similar issue [1] with the "connectionLost" method in the HTTP2 code including an additional parameter. In this case it seems to be used to pass around additional state and follows a similar pattern (of the additional parameter being prefixed with an underscore). I'll be curious what the proper way to deal with this is! --Patrick [1]: https://github.com/twisted/twisted/pull/1265#issuecomment-641512616
participants (4)
-
Craig Rodrigues
-
Glyph
-
Maarten ter Huurne
-
Patrick Cloke