Add type hints to tests/rest/client

[dklimpel]

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[DMRobertson]

Thanks for this; I'm happy if mypy is happy. A few style notes though.

Otherwise,

[DMRobertson]

We generally prefer using auto_attribs=True if possible, which allows us to use a slightly neater syntax in the class body. See for example

synapse/synapse/storage/databases/main/events_worker.py

Lines 107 to 110 in 5a69115

	@attr.s(slots=True, auto_attribs=True)
	class EventCacheEntry:
	event: EventBase
	redacted_event: Optional[EventBase]

[DMRobertson]

It doesn't make sense to invite to, leave, or ban from an unspecified room. Do you know why this annotation allowed an Optional room in the first place? Are/were there any tests taking advantage of this?

[dklimpel]

It calls change_membership.
There it is required and also needed for the path.

synapse/tests/rest/client/utils.py

Lines 252 to 283 in 2b0f157

	def change_membership(
	self,
	room: str,
	src: Optional[str],
	targ: Optional[str],
	membership: str,
	extra_data: Optional[dict] = None,
	tok: Optional[str] = None,
	appservice_user_id: Optional[str] = None,
	expect_code: int = HTTPStatus.OK,
	expect_errcode: Optional[str] = None,
	) -> None:
	"""
	Send a membership state event into a room.
	Args:
	room: The ID of the room to send to
	src: The mxid of the event sender
	targ: The mxid of the event's target. The state key
	membership: The type of membership event
	extra_data: Extra information to include in the content of the event
	tok: The user access token to use
	appservice_user_id: The `user_id` URL parameter to pass.
	This allows driving an application service user
	using an application service access token in `tok`.
	expect_code: The expected HTTP response code
	expect_errcode: The expected Matrix error code
	"""
	temp_id = self.auth_user_id
	self.auth_user_id = src
	path = f"/_matrix/client/r0/rooms/{room}/state/m.room.member/{targ}"

I think, hope it was a mistake, earlier.

[dklimpel]

It was part of the inital Homeserver commit:
4f475c7#diff-7d9372b9840620c86d8739fc6be631da0bcdcc4b41101b435dc4429ad34b6094

[DMRobertson]

I think we normally prefer assert first_event_id is not None when we're narrowing Optional[T] to T.

[DMRobertson]

Many thanks Dirk! LGTM. Once the unit tests have finished running, I'll merge this. Assuming sytest is still screwed for now.