Fix mypy error: Too few arguments to "makeTestCaseClasses" #1261

rodrigc · 2020-05-05T09:18:01Z

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

rodrigc · 2020-05-05T09:18:50Z

This helps address some of the mypy errors found here: https://travis-ci.com/github/twisted/twisted/jobs/327861205#L4888

such as:

src/twisted/internet/test/reactormixins.py:350:27: error: Incompatible types in
assignment (expression has type "classmethod", variable has type
"Callable[[ReactorBuilder], Any]")  [assignment]
        makeTestCaseClasses = classmethod(makeTestCaseClasses)

src/twisted/internet/test/test_win32events.py:199:18: error: Too few arguments
for "makeTestCaseClasses"  [call-arg]
    globals().update(Win32EventsTestsBuilder.makeTestCaseClasses())

I followed the suggestions here: https://stackoverflow.com/questions/44640479/mypy-annotation-for-classmethod-returning-instance
for using TypeVar

- Fix some pyflakes/pylint errors. - Add some type annotations to appease mypy

glyph

Aside from needing to annotating the type: ignore comments before landing, this looks great, thank you!

glyph · 2020-05-08T00:52:37Z

src/twisted/internet/test/reactormixins.py

    from twisted.internet import process
+except ImportError:
+    process = None  # type: ignore


Could you file a ticket for a more mypy-friendly way of doing this, and link it here before landing? I am OK with adding type: ignore for an import hack like this (particularly in a test helper) but before the pattern replicates ad infinitum I would like to record the fact that we should clean it up somehow later.

There already is an open issue python/mypy#1297 (comment) where this use-case was mentioned. I'll follow up there, and see what I can find out.

So based on this: python/mypy#1297 (comment)
I think we can remove the #type: ignore, and do this:

try from twisted.internet import process as _process except ImportError: process = None else: process = _process

glyph · 2020-05-08T00:53:09Z

src/twisted/internet/test/reactormixins.py

-    def makeTestCaseClasses(cls):
+    @classmethod
+    def makeTestCaseClasses(
+        cls: Type['ReactorBuilder']


Why are these all quoted? The names are imported, so they should be in scope?

ReactorBuilder is defined in the same file, so I had to use the syntax Class name forward references, as described here: https://mypy.readthedocs.io/en/stable/kinds_of_types.html#class-name-forward-references

otherwise mypy would complain.

Actually mypy did not complain. If I didn't put ReactorBuilder in quotes, pyflakes complained with:

src/twisted/internet/test/reactormixins.py:331:19 undefined name 'ReactorBuilder' src/twisted/internet/test/reactormixins.py:332:31 undefined name 'ReactorBuilder'

I've changed this so SynchronousTestCase is not in quotes, but I still need 'ReactorBuilder` to be in quotes for the annotation.

glyph · 2020-05-08T00:53:26Z

src/twisted/internet/test/reactormixins.py

        """
        Create a L{SynchronousTestCase} subclass which mixes in C{cls} for each
        known reactor and return a dict mapping their names to them.
        """
-        classes = {}
+        classes = {}  # type: Dict[str, Union[Type['ReactorBuilder'], Type['SynchronousTestCase']]]   # noqa


Quoting in type comments seems doubly pointless, even if it works?

I'm not sure exactly how mypy parses the file and comments to do all the type checks. I just kept the use the same to be consistent.

For mypy's sake you never need to quote, but the Python VM does require it when using a type name before it is defined, unless you use from __future__ import annotations, but that's a Python 3.7 feature.

Of course the Python VM doesn't process the contents of comments, so no quoting is necessary there.

@mthuurne Yes, for now I need to quote 'ReactorBuilder'. Since Twisted still supports Python 3.5 and 3.6, I cannot use from __future__ import annotations

glyph · 2020-05-08T00:53:43Z

src/twisted/internet/test/reactormixins.py

        for reactor in cls._reactors:
            shortReactorName = reactor.split(".")[-1]
-            name = (cls.__name__ + "." + shortReactorName + "Tests").replace(".", "_")
-            class testcase(cls, SynchronousTestCase):
+            name = (cls.__name__ + "." + shortReactorName +


I guess twistedchecker complained at you?

Yes, I made a lot of small changes like this to appease twistedchecker in terms of line spacing, line length, etc.

glyph · 2020-05-08T00:54:00Z

src/twisted/internet/test/reactormixins.py

+            name = (cls.__name__ + "." + shortReactorName +
+                    "Tests").replace(".", "_")
+
+            class testcase(cls, SynchronousTestCase):  # type: ignore


What doesn't mypy like about this? Again, I would like type: ignores to be used sparingly and we should generally have a plan for how to get rid of them, at least file an upstream bug against mypy.

If I didn't put that #type: ignore there, mypy would give:

src/twisted/internet/test/reactormixins.py:343:28: error: Variable "cls" is not valid as a type [valid-type] class testcase(cls, SynchronousTestCase): ^ src/twisted/internet/test/reactormixins.py:343:28: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases src/twisted/internet/test/reactormixins.py:343:28: error: Invalid base class "cls" [misc] class testcase(cls, SynchronousTestCase):

The reason for this is that cls is a parameter, and the type of the parameter is only known at runtime. So mypy doesn't know what type it is, and can't check it.

Note that if you have to use type: ignore, you can specify the error category to ignore, which reduces the chance of accidentally silencing unexpected errors. In this case it would be # type: ignore[valid-type].

@mthuurne thanks for the feedback. I've changed this line to specifically turn off two mypy errors on this line:

class testcase(cls, SynchronousTestCase): # type: ignore[valid-type,misc]

glyph · 2020-05-08T00:54:39Z

src/twisted/internet/test/reactormixins.py

                __module__ = cls.__module__
                if reactor in cls.skippedReactors:
                    skip = cls.skippedReactors[reactor]
                try:
                    reactorFactory = namedAny(reactor)
-                except:
+                except Exception:


This should be BaseException to avoid a change to its semantics.

…asses. See: https://mypy.readthedocs.io/en/stable/kinds_of_types.html#class-name-forward-references

rodrigc requested review from adiroiban, wsanchez and glyph May 5, 2020 09:19

rodrigc force-pushed the 9811-rodrigc-mypy branch 5 times, most recently from 1139eb9 to 1efd196 Compare May 5, 2020 17:22

rodrigc requested a review from hawkowl May 7, 2020 20:53

rodrigc added 2 commits May 7, 2020 14:45

Use @classmethod decorator for makeTestCaseClasses()

1e43d90

- Fix some pyflakes/pylint errors. - Add some type annotations to appease mypy

Add newsfragment

a123fda

rodrigc force-pushed the 9811-rodrigc-mypy branch from 1efd196 to a123fda Compare May 7, 2020 21:45

glyph approved these changes May 8, 2020

View reviewed changes

Change to BaseException

e4989c3

rodrigc mentioned this pull request May 8, 2020

Issues with conditional imports python/mypy#1297

Closed

rodrigc added 3 commits May 7, 2020 19:18

ReactorBuilder must be in quotes in the annotation for makeTestCaseCl…

98769fe

…asses. See: https://mypy.readthedocs.io/en/stable/kinds_of_types.html#class-name-forward-references

Rework conditional import to work with mypy

a54deeb

Turn off specific mypy errors on this line

010e744

rodrigc force-pushed the 9811-rodrigc-mypy branch from 44f91b7 to 010e744 Compare May 8, 2020 03:38

rodrigc merged commit 79a26b0 into trunk May 8, 2020

rodrigc deleted the 9811-rodrigc-mypy branch May 8, 2020 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mypy error: Too few arguments to "makeTestCaseClasses" #1261

Fix mypy error: Too few arguments to "makeTestCaseClasses" #1261

rodrigc commented May 5, 2020

rodrigc commented May 5, 2020 •

edited

Loading

glyph left a comment

glyph May 8, 2020

rodrigc May 8, 2020

rodrigc May 8, 2020 •

edited

Loading

glyph May 8, 2020

rodrigc May 8, 2020

rodrigc May 8, 2020 •

edited

Loading

rodrigc May 8, 2020

glyph May 8, 2020

rodrigc May 8, 2020

mthuurne May 8, 2020 •

edited

Loading

rodrigc May 8, 2020

glyph May 8, 2020

rodrigc May 8, 2020

glyph May 8, 2020

rodrigc May 8, 2020

mthuurne May 8, 2020

rodrigc May 8, 2020

glyph May 8, 2020

Fix mypy error: Too few arguments to "makeTestCaseClasses" #1261

Fix mypy error: Too few arguments to "makeTestCaseClasses" #1261

Conversation

rodrigc commented May 5, 2020

rodrigc commented May 5, 2020 • edited Loading

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc May 8, 2020 • edited Loading

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

mthuurne May 8, 2020 • edited Loading

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

rodrigc commented May 5, 2020 •

edited

Loading

rodrigc May 8, 2020 •

edited

Loading

rodrigc May 8, 2020 •

edited

Loading

mthuurne May 8, 2020 •

edited

Loading