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

Factor out MultiWriter token from RoomStreamToken #16427

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

erikjohnston
Copy link
Member

So we can reuse code when we split up other streams.

@@ -60,6 +61,8 @@
from synapse.util.stringutils import parse_and_validate_server_name

if TYPE_CHECKING:
from typing_extensions import Self
Copy link
Member Author

Choose a reason for hiding this comment

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

This requires typing_extensions >= 4.0. Not sure how we feel about bumping that? Or just leaving it behind TYPE_CHECKING?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy as long as the packagers are. https://pkgs.org/search/?q=typing-extensions and https://repology.org/project/python:typing-extensions/versions. Debian buster and Ubuntu focal + jammy have 3.x; I can't remember if we've dropped support for those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ISWYM. It'd be nice to be able to just import it, but I don't mind the conditional import and referencing it in "quotes" if that helps the packagers too. (ISTR we have to quote "defer.Deferred[blah]" anyway...

Copy link
Member

Choose a reason for hiding this comment

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

I think putting it behind TYPE_CHECKING is fine.

@erikjohnston erikjohnston marked this pull request as ready for review October 4, 2023 14:45
@erikjohnston erikjohnston requested a review from a team as a code owner October 4, 2023 14:45
synapse/types/__init__.py Outdated Show resolved Hide resolved
value_validator=attr.validators.instance_of(int),
mapping_validator=attr.validators.instance_of(immutabledict),
),
kw_only=True,
Copy link
Member

Choose a reason for hiding this comment

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

OMG can we use kw_only everywhere?!?! 😍

Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Seems legit if CI is green.

@clokep
Copy link
Member

clokep commented Oct 4, 2023

sytest + workers failing scares me a bit though...

@erikjohnston erikjohnston merged commit 009b47b into develop Oct 5, 2023
38 checks passed
@erikjohnston erikjohnston deleted the erikj/factor_out_multiwriter branch October 5, 2023 09:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants