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

Add type hints to synapse.events. #10998

Closed
wants to merge 18 commits into from
Closed

Add type hints to synapse.events. #10998

wants to merge 18 commits into from

Conversation

clokep
Copy link
Member

@clokep clokep commented Oct 5, 2021

Adds missing type hints to the synapse.events module and ensures mypy checks them.

There are still some missing type hints around the DictProperty field (and EventBase, kind of). There's some deep magic there and my attempts to add type hints caused many errors throughout the code-base. I think doing that separately is probably desired.

@clokep clokep force-pushed the clokep/event-types branch from c5cfdc2 to 708c163 Compare October 5, 2021 18:17
@clokep clokep requested a review from a team October 5, 2021 18:51
@babolivier babolivier self-assigned this Oct 6, 2021
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Mostly just general comments, but the V2/V3 typo needs a fix I think!

clokep and others added 2 commits October 6, 2021 08:14
Comment on lines +1496 to +1501
conflicting_insertion_event_id = None
if next_batch_id:
conflicting_insertion_event_id = (
await self.store.get_insertion_event_by_batch_id(
event.room_id, next_batch_id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance, this looks like it might be a bug fix; does something bad happen if we pass next_batch_id=None to get_insertion_event_by_batch_id? Or perhaps the preceeding code was such that next_batch_id was never None, and we just need to convince mypy of that?

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

LGTM as a first step, assuming CI passes! I'd be happy for you to get this merged in as it is now and work on a part 2, as we mentioned in #synapse-dev.

@clokep
Copy link
Member Author

clokep commented Oct 12, 2021

I'm finding this PR unwieldy. I'm going to split it in half.

@clokep clokep closed this Oct 12, 2021
@clokep clokep deleted the clokep/event-types branch October 14, 2021 17:43
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