Add type annotations to _pytest.compat and _pytest._code.code by bluetech · Pull Request #6205 · pytest-dev/pytest

bluetech · 2019-11-16T14:25:01Z

This PR starts with some small prerequisite commits, and then adds type annotations to _pytest.compat and _pytest._code.code.

Let me know if this is too big for one PR, and I will split it. Note that some part of the diff is just adding -> None to some test functions, so that mypy will check them without check_untyped_defs = True (which needs more work).

Add some Python 3.8 type: ignores; all are already fixed in the next mypy release, so can be removed once we upgrade. Also move some flake8 ignores which seem to have changed places.

pytest doesn't support these PyPy versions anymore, so no need to have checks for them.

It doesn't help much IMO, just adds indirection and makes it harder to type.

The previous test was better in that it used fakes to test all of the real code paths. The problem with that is that it makes it impossible to simplify the code with `isinstance` checks. So let's just simulate the issue directly with a monkeypatch.

Source was previously iterable because it implements `__getitem__()`, which is apparently a thing from before `__iter__()` was introduced. To reduce mypy's and my own confusion, implement `__iter__()` directly.

bluetech · 2019-11-16T15:27:04Z

src/_pytest/_code/source.py


    @overload  # noqa: F811
-    def __getitem__(self, key: slice) -> "Source":
+    def __getitem__(self, key: slice) -> "Source":  # noqa: F811


There seems to be a discrepancy between where CI flake8 and my flake8 report this - on the decorator or the def. Maybe because of Python 3.8? So for now I add it to both.

Maybe because of Python 3.8?

Very likely, yes.
Isn't there a flake8 plugin to handle this automatically maybe?

There seems to be some discussion regarding this here, I'm too lazy to read through it though...

blueyed

Awesome!

blueyed · 2019-11-16T16:51:11Z

src/_pytest/_code/code.py

@@ -38,13 +46,12 @@ class Code:
    def __init__(self, rawcode) -> None:


Can we type the rawcode arg here?

Not really. I spent some time trying to type getrawcode but left it aside for now.

blueyed · 2019-11-16T16:52:05Z

src/_pytest/_code/code.py

        return eval(code, self.f_globals, f_locals)

-    def exec_(self, code, **vars):
+    def exec_(self, code, **vars) -> None:


Can the args be typed here also?

They can be anything really AFAIK, but I'm not entirely sure, so I left it out.

blueyed · 2019-11-16T16:56:08Z

src/_pytest/_code/code.py

-
-    def __init__(self, tb, excinfo=None):
+    def __init__(
+        self, tb: Union[TracebackType, Iterable[TracebackEntry]], excinfo=None


Can excinfo be typed?

Yes, will add.

blueyed · 2019-11-16T16:56:58Z

src/_pytest/_code/code.py

+        path=None,
+        lineno: Optional[int] = None,
+        firstlineno: Optional[int] = None,
+        excludepath=None,


Can excludepath be typed?

It's can be a py.path AFAIU, so I'm leaving it unannotated.

blueyed · 2019-11-16T17:04:23Z

src/_pytest/_code/code.py

+            # Type ignored because mypy thinks the slice is a List[TracebackEntry]
+            # instead of the Traceback subclass, even though we override __getitem__().
+            # Should figure out why it does that.
+            traceback = traceback[:max_frames] + traceback[-max_frames:]  # type: ignore


The slice is a Traceback, but only the expression traceback[:max_frames] + traceback[-max_frames:] is a List[…] then.
No idea how to fix it, but it might help you.. :)

Right, I misunderstood the situation. Looks like a deficiency in mypy/typeshed for now. I'll update the comment.

blueyed · 2019-11-16T17:08:37Z

src/_pytest/_code/code.py

+        reprfuncargs: Optional["ReprFuncArgs"],
+        reprlocals: Optional["ReprLocals"],
+        filelocrepr: Optional["ReprFileLocation"],
+        style: str,


Literal for style?

Yes, will add.

blueyed · 2019-11-16T17:09:17Z

src/_pytest/_code/code.py


    def toterminal(self, tw) -> None:
        if self.style == "short":
+            assert self.reprfileloc is not None


Could (additionally) be checked/validated in __init__? But it's fine like that also.

Wouldn't happen unless the code is buggy; seems fine to only do it here.

blueyed · 2019-11-16T17:11:04Z

src/_pytest/assertion/rewrite.py

 def get_cache_dir(file_path: Path) -> Path:
    """Returns the cache directory to write .pyc files for the given .py file path"""
-    if sys.version_info >= (3, 8) and sys.pycache_prefix:
+    # Type ignored until added in next mypy release.


Would be good to link an issue in general, but ok for now.

mypy can detect outdated type: ignores so it's easy to get rid of when updating mypy.

blueyed · 2019-11-16T17:16:05Z

src/_pytest/compat.py

-        warnings.warn(FUNCARGNAMES, stacklevel=2)
-        return self.fixturenames
-
-


I'm ok with inlining, but wondered how it made typing harder (from the commit message)?

It's a mixin class, it uses fixturenames which is not defined in the class so mypy gets confused. There are ways to type this but it's not worth the bother in this case IMO.

blueyed · 2019-11-16T17:21:48Z

testing/code/test_source.py

    assert ex.value.lineno == 1
    assert ex.value.offset in {5, 7}  # cpython: 7, pypy3.6 7.1.1: 5
+    assert ex.value.text is not None
    assert ex.value.text.strip(), "x x"


Could check the full expected text instead?
assert ex.value.text == "xyz xyz\n"

Yes, that's better.

blueyed · 2019-11-16T17:24:55Z

Note that some part of the diff is just adding -> None to some test functions, so that mypy will check them without check_untyped_defs = True (which needs more work).

IIRC this (check_untyped_defs) could be enabled based on modules also?

bluetech · 2019-11-16T20:34:34Z

IIRC this (check_untyped_defs) could be enabled based on modules also?

Yes, that's an option to. The -> None is not too bad, though.

bluetech · 2019-11-16T20:38:19Z

Thanks for the review @blueyed, updated.

blueyed

👍

bluetech added 8 commits November 16, 2019 17:22

A few linting fixes

f760356

Add some Python 3.8 type: ignores; all are already fixed in the next mypy release, so can be removed once we upgrade. Also move some flake8 ignores which seem to have changed places.

Remove a PyPy version check for an unsupported version

c7a83a0

pytest doesn't support these PyPy versions anymore, so no need to have checks for them.

Remove checks for Python2-only fields im_func and func_code

04d68fb

Remove unneeded getrawcode() calls from tests

5bfe793

Inline the FuncargnamesCompatAttr compat helper

e3ac44d

It doesn't help much IMO, just adds indirection and makes it harder to type.

Simplify a FormattedExcinfo test

307add0

The previous test was better in that it used fakes to test all of the real code paths. The problem with that is that it makes it impossible to simplify the code with `isinstance` checks. So let's just simulate the issue directly with a monkeypatch.

Make Source explicitly implement __iter__()

a649f15

Source was previously iterable because it implements `__getitem__()`, which is apparently a thing from before `__iter__()` was introduced. To reduce mypy's and my own confusion, implement `__iter__()` directly.

Add type annotations to _pytest.compat

562d481

bluetech force-pushed the type-annotations-8 branch from b512b9c to 51e3be2 Compare November 16, 2019 15:25

bluetech commented Nov 16, 2019

View reviewed changes

blueyed reviewed Nov 16, 2019

View reviewed changes

Add type annotations to _pytest._code.code

eaa34a9

bluetech force-pushed the type-annotations-8 branch from 51e3be2 to eaa34a9 Compare November 16, 2019 20:36

blueyed approved these changes Nov 16, 2019

View reviewed changes

bluetech merged commit fa578d7 into pytest-dev:features Nov 17, 2019

bluetech deleted the type-annotations-8 branch November 17, 2019 07:45

		@@ -38,13 +46,12 @@ class Code:
		def __init__(self, rawcode) -> None:

		warnings.warn(FUNCARGNAMES, stacklevel=2)
		return self.fixturenames

Uh oh!

Conversation

bluetech commented Nov 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed commented Nov 16, 2019

Uh oh!

bluetech commented Nov 16, 2019

Uh oh!

bluetech commented Nov 16, 2019

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants