search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
160 stars 144 forks source link

Add type hints to webpushpushkin.py #271

Closed H-Shay closed 2 years ago

H-Shay commented 2 years ago

Hopefully this is a little easier to parse. Just one file. There are a few mypy errors but I believe they will be resolved once the type hints for notifications.py are accepted and merged.

H-Shay commented 2 years ago

Mypy appears to be unhappy:

So I mentioned this at the top of the PR that I think these errors will be resolved once the changes to notifications.py are merged-is the best thing to do to wait until that happens? Just wasn't sure how to manage the dependencies amongst mypy'd files in separate PR's.

DMRobertson commented 2 years ago

Mypy appears to be unhappy:

So I mentioned this at the top of the PR that I think these errors will be resolved once the changes to notifications.py are merged-is the best thing to do to wait until that happens? Just wasn't sure how to manage the dependencies amongst mypy'd files in separate PR's.

Arg, sorry. You did indeed; I just dived straight into the diff. I think the changes you refer to are in https://github.com/matrix-org/sygnal/pull/264 ?

FWIW, I've found this workflow to be okayish for managing this kind of dependency:

I've handled this in the past by starting a second branch based on the first change's branch. Then I target the PR at main/develop/master and mark it as a draft. That means that the PR includes both commits from the first and second branch, so I note in the description "depends on #<number>, first interesting commit is <hash>".

I typically end up

squahtx commented 2 years ago

LGTM. Maybe @squahtx wants to have their say too? Otherwise, I think this is good to go once #264 is merged.

It all looks good to me!

(I wonder if it's tractable to change Dict[str, Any] to Dict[str, object] so that mypy makes us add validation. But that's something for another time.)