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

Fix type hint errors on Twisted trunk #16526

Merged
merged 4 commits into from
Oct 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/16526.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve type hints.
16 changes: 11 additions & 5 deletions synapse/util/file_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

import queue
from typing import BinaryIO, Optional, Union, cast
from typing import Any, BinaryIO, Optional, Union, cast

from twisted.internet import threads
from twisted.internet.defer import Deferred
Expand Down Expand Up @@ -58,7 +58,9 @@ def __init__(self, file_obj: BinaryIO, reactor: ISynapseReactor) -> None:
self._bytes_queue: queue.Queue[Optional[bytes]] = queue.Queue()

# Deferred that is resolved when finished writing
self._finished_deferred: Optional[Deferred[None]] = None
#
# This is really Deferred[None], but mypy doesn't seem to like that.
self._finished_deferred: Optional[Deferred[Any]] = None

# If the _writer thread throws an exception it gets stored here.
self._write_exception: Optional[Exception] = None
Expand All @@ -80,9 +82,13 @@ def registerProducer(
self.streaming = streaming
self._finished_deferred = run_in_background(
threads.deferToThreadPool,
self._reactor,
self._reactor.getThreadPool(),
self._writer,
# mypy seems to get confused with the chaining of ParamSpec from
# run_in_background to deferToThreadPool.
#
# For Twisted trunk, ignore arg-type; for Twisted release ignore unused-ignore.
self._reactor, # type: ignore[arg-type,unused-ignore]
self._reactor.getThreadPool(), # type: ignore[arg-type,unused-ignore]
self._writer, # type: ignore[arg-type,unused-ignore]
Comment on lines +89 to +91
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really like this, but I don't see another option for getting this to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that these ignores are here for Twisted release, the Twisted Trunk job ignore unused ignores.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this, but I don't see another option for getting this to work?

Let's roll with this. (ParamSpec chaining has been a source of pain for me, too. I think it tends not to go well if you pass it into an overloaded function?)

)
if not streaming:
self._producer.resumeProducing()
Expand Down
1 change: 1 addition & 0 deletions tests/handlers/test_appservice.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ def test_query_room_alias_exists(self) -> None:
result = self.successResultOf(
defer.ensureDeferred(self.handler.query_room_alias_exists(room_alias))
)
assert result is not None

self.mock_as_api.query_alias.assert_called_once_with(
interested_service, room_alias_str
Expand Down
2 changes: 1 addition & 1 deletion tests/http/server/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ def __init__(self, seen_awaits: Set[Tuple[str, ...]], request_number: int):
self._request_number = request_number
self._seen_awaits = seen_awaits

self._original_Deferred___next__ = Deferred.__next__
self._original_Deferred___next__ = Deferred.__next__ # type: ignore[misc,unused-ignore]
Copy link
Contributor

@DMRobertson DMRobertson Oct 23, 2023

Choose a reason for hiding this comment

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

What's the misc complaint about OOI?

Copy link
Member Author

Choose a reason for hiding this comment

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

tests/http/server/_base.py:338: error: Access to generic instance variables via class is ambiguous  [misc]

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is complaining about snagging a instance level method from a class?


# The number of `await`s on `Deferred`s we have seen so far.
self.awaits_seen = 0
Expand Down
2 changes: 1 addition & 1 deletion tests/http/test_matrixfederationclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def test_client_get(self) -> None:
"""

@defer.inlineCallbacks
def do_request() -> Generator["Deferred[object]", object, object]:
def do_request() -> Generator["Deferred[Any]", object, object]:
with LoggingContext("one") as context:
fetch_d = defer.ensureDeferred(
self.cl.get_json("testserv:8008", "foo/bar")
Expand Down
3 changes: 2 additions & 1 deletion tests/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
Generic,
Iterable,
List,
Mapping,
NoReturn,
Optional,
Tuple,
Expand Down Expand Up @@ -251,7 +252,7 @@ def assertObjectHasAttributes(self, attrs: Dict[str, object], obj: object) -> No
except AssertionError as e:
raise (type(e))(f"Assert error for '.{key}':") from e

def assert_dict(self, required: dict, actual: dict) -> None:
def assert_dict(self, required: Mapping, actual: Mapping) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Surprised twisted picked this up. Is the point that some twisted function now returns a proper type, so we get a Mapping instead of an Any?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, pretty much. 👍

"""Does a partial assert of a dict.

Args:
Expand Down
Loading