-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
c5cfdc2
to
708c163
Compare
There was a problem hiding this 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!
Co-authored-by: Brendan Abolivier <[email protected]>
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 | ||
) |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
I'm finding this PR unwieldy. I'm going to split it in half. |
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 (andEventBase
, 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.