Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Add missing type hints for tests.unittest. #13397

Merged
clokep merged 11 commits into from
Jul 27, 2022
Merged

Add missing type hints for tests.unittest. #13397

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1674b7f
Add missing type hints for override_config.
clokep Jul 26, 2022
6c0344a
Add type hints to prepare.
clokep Jul 26, 2022
e36b12d
Add type hints to assert_dict.
clokep Jul 26, 2022
e4012f0
Add type hints to around.
clokep Jul 26, 2022
4853dd5
Add type hints to logging methods.
clokep Jul 26, 2022
76270c3
Add missing type hints.
clokep Jul 26, 2022
5a21ef7
Newsfragment
clokep Jul 26, 2022
700dfcf
Lint
clokep Jul 26, 2022
aca8cf6
Add a separate json_list method on FakeChannel.
clokep Jul 27, 2022
603d70a
Additional corrections from review.
clokep Jul 27, 2022
74d6acb
Use another typevar.
clokep Jul 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions tests/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import canonicaljson
import signedjson.key
import unpaddedbase64
from typing_extensions import Protocol
from typing_extensions import ParamSpec, Protocol

from twisted.internet.defer import Deferred, ensureDeferred
from twisted.python.failure import Failure
Expand Down Expand Up @@ -88,6 +88,9 @@
TV = TypeVar("TV")
_ExcType = TypeVar("_ExcType", bound=BaseException, covariant=True)

P = ParamSpec("P")
R = TypeVar("R")


class _TypedFailure(Generic[_ExcType], Protocol):
"""Extension to twisted.Failure, where the 'value' has a certain type."""
Expand All @@ -97,7 +100,7 @@ def value(self) -> _ExcType:
...


def around(target):
def around(target: TV) -> Callable[[Callable[P, R]], None]:
"""A CLOS-style 'around' modifier, which wraps the original method of the
given instance with another piece of code.

Expand All @@ -106,11 +109,11 @@ def method_name(orig, *args, **kwargs):
return orig(*args, **kwargs)
"""

def _around(code):
def _around(code: Callable[P, R]) -> None:
name = code.__name__
orig = getattr(target, name)

def new(*args, **kwargs):
def new(*args: P.args, **kwargs: P.kwargs) -> R:
return code(orig, *args, **kwargs)

setattr(target, name, new)
Copy link
Contributor

@DMRobertson DMRobertson Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor nitpick: Is this strictly correct? From the perspective of type annotations only:

  • code is the wrapper function which is being decorated by @around(target).
  • The first argument to code is orig.
  • Therefore the first argument of new is also orig.
  • Therefore the call to code(...) on line +117 passes in orig twice. But that's not what the source code does.

I think code: Callable[Concatenate[object, P], R] might be more accurate (where object represents orig).

Copy link
Member Author

@clokep clokep Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be more accurate, yes. I'll check that!

Copy link
Member Author

@clokep clokep Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore the first argument of new is also orig.

I don't think this is correct -- the code would fail in this case because setUp doesn't accept *args.

new needs to match the signature of orig, not of code. I believe this is what the current code does.

Copy link
Contributor

@DMRobertson DMRobertson Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're agreeing with each other, but I didn't express myself well. Perhaps instead of

Therefore the first argument of new is also orig.

I should have said

mypy can deduce that the first argument of new has type type(orig).

Putting that aside for the moment, however, I entirely agree with your summary here:

new needs to match the signature of orig, not of code

but I don't think this is what the current set of annotations mean. At present, both new and code are of type Callable[P, R]. But this doesn't make sense, because code takes orig as an extra argument!

I still think that code: Callable[Concatenate[object, P], R] is correct here. Have I managed to convince you?

(You could try writing/casting orig: Callable[P, R] = ...; I think mypy would detect the inconsistency I've tried to describe above.)

Copy link
Member Author

@clokep clokep Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe 603d70a fixes this?

Expand All @@ -131,7 +134,7 @@ def __init__(self, methodName: str):
level = getattr(method, "loglevel", getattr(self, "loglevel", None))

@around(self)
def setUp(orig):
def setUp(orig: Callable[[], R]) -> R:
# if we're not starting in the sentinel logcontext, then to be honest
# all future bets are off.
if current_context():
Expand All @@ -144,7 +147,7 @@ def setUp(orig):
if level is not None and old_level != level:

@around(self)
def tearDown(orig):
def tearDown(orig: Callable[[], R]) -> R:
ret = orig()
logging.getLogger().setLevel(old_level)
return ret
Expand All @@ -158,7 +161,7 @@ def tearDown(orig):
return orig()

@around(self)
def tearDown(orig):
def tearDown(orig: Callable[[], R]) -> R:
ret = orig()
# force a GC to workaround problems with deferreds leaking logcontexts when
# they are GCed (see the logcontext docs)
Expand Down