[mypy] Implement some functions defined in zope interfaces #1298

rodrigc · 2020-06-19T07:25:01Z

https://twistedmatrix.com/trac/ticket/9852

When running mypy over the twisted codebase with:

tox -e mypy

There are many errors that look like this:

src/twisted/words/protocols/jabber/sasl_mechanisms.py:47:1: error: 'Anonymous' is missing following 'ISASLMechanism' interface members: getResponse.  [misc]
src/twisted/words/protocols/jabber/sasl_mechanisms.py:61:1: error: 'Plain' is missing following 'ISASLMechanism' interface members: getResponse.  [misc]
src/twisted/internet/_dumbwin32proc.py:110:1: error: 'Process' is missing following 'twisted.internet.interfaces.ITransport' interface members: getHost, getPeer.
src/twisted/internet/abstract.py:151:1: error: 'FileDescriptor' is missing following 'ITransport' interface members: getHost, getPeer.  [misc]
src/twisted/internet/process.py:623:1: error: 'Process' is missing following 'twisted.internet.interfaces.ITransport' interface members: getHost, getPeer.  [misc]
src/twisted/internet/process.py:959:1: error: 'PTYProcess' is missing following 'twisted.internet.interfaces.ITransport' interface members: getHost, getPeer.  [misc]
src/twisted/internet/process.py:959:1: error: 'PTYProcess' is missing following 'IProcessTransport' interface members: closeChildFD, writeToChild.  [misc]
src/twisted/internet/_resolver.py:31:1: error: 'HostResolution' is missing following 'IHostResolution' interface members: cancel.  [misc]
src/twisted/internet/base.py:504:1: error: 'ReactorBase' is missing following 'IReactorCore' interface members: run.  [misc]
src/twisted/internet/_newtls.py:162:1: error: 'ConnectionMixin' is missing following 'twisted.internet.interfaces.ITCPTransport' interface members: getHost, getPeer,

In an effort to quieten these errors, this
PR adds comments to turn off these mypy warnings in a few places.

In a few other places, I implemented no-op methods in some classes, to appease mypy.
add comments

adiroiban

Thanks for the changes.

This is an easy to review chunk of changes :)

I am not sure what are the rules for returning None or raising a NotImplementedError from the stub methods.

I don't see the value of # type: ignore[misc].
We should either work at defining the proper types , or not using types at all.

adiroiban · 2020-06-19T10:26:36Z

src/twisted/application/runner/test/test_pidfile.py

+        """
+        @see: L{IFilePath.sep}
+
+        Not implemented


shouldn't this raise NotImplemented error?
In the case in which some new test is using it, it will raise an explicit error, rather than a generic error about the None type missing some string attributes.

Also I think it's better to have no docstring rather than one containing only @see, since if there is no docstring, pydoctor will inherit the docstring from the superclass/interface.

@mthuurne I put the @see docstring in there, because when looking directly at the code, it is sometimes difficult to figure out what method comes from what interface. I can take out the docstring though if people don't want it.

I guess for test code it doesn't matter much, but for modules that end users are going to be reading the documentation of, I think it's better to have the inherited documentation inlined by pydoctor, since that means it is accessible without having to navigate to a different page when reading the docs as HTML.

src/twisted/conch/insults/helper.py

src/twisted/internet/_newtls.py

src/twisted/internet/unix.py

adiroiban · 2020-06-19T10:45:06Z

src/twisted/mail/imap4.py

+        """
+        @see: L{INamespacePresenter.getUserNamespaces}
+        """
+        return None


I know that in the docstring we have

If no namespaces of this type exist, None should be returned.

But what would happen if we return an empty tuple here?

It will be a sequence and the method will always return the same type.

I'm not sure. Maybe this interface should have been implemented to return an empty tuple instead of None, but I didn't want to change the semantics of the function, as defined by the interface.

True. Thanks. This should have the optional type for sure. Sorry for the noise.

adiroiban · 2020-06-19T11:02:22Z

src/twisted/python/test/test_components.py

+class Yayable(object):  # type: ignore[misc]
+    # class does not implement Attribute ifaceAttribute
+    # so we need to turn off mypy warning


Suggested change

class Yayable(object): # type: ignore[misc]

# class does not implement Attribute ifaceAttribute

# so we need to turn off mypy warning

class Yayable(object):

# This is replaced by each test. Here we add a default value.

ifaceAttribute = object()

@adiroiban I made the change you suggested, but then the test failed with:

[FAIL] Traceback (most recent call last): File "/Users/crodrigues/Twisted/src/twisted/python/test/test_components.py", line 907, in test_attributeCustomization self.assertFalse(hasattr(yayable, 'ifaceAttribute')) File "/Users/crodrigues/Twisted/src/twisted/trial/_synctest.py", line 390, in assertFalse super(_Assertions, self).assertFalse(condition, msg) File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 699, in assertFalse raise self.failureException(msg) twisted.trial.unittest.FailTest: True is not false

So the test explicitly checks that ifaceAttribute is not there, before adding that attribute.

I'm not sure if it is OK to change the test?

I think that we can change the tests as the 'hasAttr' check is not 100% correct.

As we have seen here, even if the instance member is removed, hasAttr will still return True...as the class has the attrribute.

I have proposed a patch to improve this test and make it explicit that only the instance attribute is removed.

adiroiban · 2020-06-20T07:06:22Z

src/twisted/python/test/test_components.py

+class Yayable(object):  # type: ignore[misc]
+    # class does not implement Attribute ifaceAttribute
+    # so we need to turn off mypy warning


It looks like ifaceAttribute was left out on purpose... but I think that we should have a comment to make it clear why the interface implementation is broken.

Suggested change

class Yayable(object): # type: ignore[misc]

# class does not implement Attribute ifaceAttribute

# so we need to turn off mypy warning

class Yayable(object): # type: ignore[misc]

"""

This class does not defines the required ifaceAttribute so that it can be used

to test that proxied attributes can also be removed.

"""

So it should have an ignore anyway, but I think that the test can be extended to be explicit and also demonstrate that no wizardry is done and that only the instance attribute is removed.

something like this

diff --git a/src/twisted/python/test/test_components.py b/src/twisted/python/test/test_components.py index 17a571c47..bef7e31d8 100644 --- a/src/twisted/python/test/test_components.py +++ b/src/twisted/python/test/test_components.py @@ -571,6 +571,12 @@ class IProxiedInterface(Interface): ifaceAttribute = Attribute(""" An example declared attribute, which should be proxied.""") + + deleteAttribute = Attribute(""" + Proxied attribute used in the test checking the attribute + delete operation.""") + + def yay(*a, **kw): """ A sample method which should be proxied. @@ -593,10 +599,16 @@ class Yayable(object): A provider of L{IProxiedInterface} which increments a counter for every call to C{yay}. + `deleteAttribute` is not implemented on purpose to support the delete + tests. + @ivar yays: The number of times C{yay} has been called. """ + ifaceAttribute = 'class-value' + def __init__(self): + self.ifaceAttribute = 'instance-value' self.yays = 0 self.yayArgs = [] @@ -743,12 +755,17 @@ class ProxyForInterfaceTests(unittest.SynchronousTestCase): """ The attributes that proxy objects proxy should be deletable and affect the original object. + + It only removes the instance attribute. """ yayable = Yayable() - yayable.ifaceAttribute = None + yayable.ifaceAttribute = 'instance' + yayable.deleteAttribute = 'something' proxy = proxyForInterface(IProxiedInterface)(yayable) del proxy.ifaceAttribute - self.assertFalse(hasattr(yayable, 'ifaceAttribute')) + del proxy.deleteAttribute + self.assertEqual('class-value', yayable.ifaceAttribute) + self.assertFalse(hasattr(yayable, 'deleteAttribute')) def test_multipleMethods(self): @@ -894,4 +911,4 @@ class ProxyForInterfaceTests(unittest.SynchronousTestCase): proxy.ifaceAttribute = thingy self.assertIs(yayable.ifaceAttribute, thingy) del proxy.ifaceAttribute - self.assertFalse(hasattr(yayable, 'ifaceAttribute')) + self.assertEqual('class-value', proxy.ifaceAttribute)

adiroiban

all good. thanks.
only minor comments.

For public API I also prefer not to have any docstring at all and let pydoctor do its magic.

For test code, rather than "@see: L{IListeningPort.getHost}" I prefer a comment explaining that this is just a stub implementation created to satisfy the interface.

With an IDE you already get the @see functionality.

This reduces errors from mypy

rodrigc requested a review from a team June 19, 2020 07:25

adiroiban reviewed Jun 19, 2020

View reviewed changes

rodrigc mentioned this pull request Jun 19, 2020

Eliminate more mypy errors #1294

Merged

rodrigc force-pushed the 9852-rodrigc-mypy branch from 49346be to 4e3a4ab Compare June 19, 2020 22:33

adiroiban reviewed Jun 20, 2020

View reviewed changes

adiroiban approved these changes Jun 20, 2020

View reviewed changes

rodrigc added 4 commits June 20, 2020 18:05

Add newsfragment

d18d357

Implement methods defined in zope interfaces

cf4523f

This reduces errors from mypy

Implement methods defined in zope interfaces

12d3430

This reduces errors from mypy

Make upDownError() signature match the one in IReporter interface

5c6fa25

rodrigc force-pushed the 9852-rodrigc-mypy branch from 4e3a4ab to 5c6fa25 Compare June 21, 2020 03:06

rodrigc merged commit 066b440 into trunk Jun 21, 2020

rodrigc deleted the 9852-rodrigc-mypy branch June 21, 2020 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mypy] Implement some functions defined in zope interfaces #1298

[mypy] Implement some functions defined in zope interfaces #1298

rodrigc commented Jun 19, 2020

adiroiban left a comment

adiroiban Jun 19, 2020

mthuurne Jun 19, 2020

rodrigc Jun 19, 2020

mthuurne Jun 20, 2020

adiroiban Jun 19, 2020

rodrigc Jun 19, 2020

adiroiban Jun 20, 2020

adiroiban Jun 19, 2020

rodrigc Jun 19, 2020

adiroiban Jun 20, 2020

adiroiban Jun 20, 2020

adiroiban left a comment

[mypy] Implement some functions defined in zope interfaces #1298

[mypy] Implement some functions defined in zope interfaces #1298

Conversation

rodrigc commented Jun 19, 2020

adiroiban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment