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

An attempt to type tests.util.caches #15032

Closed
wants to merge 7 commits into from

Conversation

DMRobertson
Copy link
Contributor

This wasn't very satisfactory: I had to resort to a lot of type-ignores here.

I struggle to understand our caching descriptor machinery at the best of times, but I think that the annotations for @cached, plus whatever we do in the Synapse mypy plugin isn't quite working right in these tests.

It's not chopped up into meaningful commits; I am happy to rebase this to do so if it would be helpful.

LMK what you think. Is this good enough to land, or should we look for a different approach?

@DMRobertson
Copy link
Contributor Author

should we look for a different approach?

I think sticking some defer.ensureDeferred might remove the need for type-ignores---though it's still about the same amount of effort to make mypy accept runtime-valid source code.

@DMRobertson DMRobertson marked this pull request as ready for review February 9, 2023 14:00
@DMRobertson DMRobertson requested a review from a team as a code owner February 9, 2023 14:00
Comment on lines +213 to +214
# type-ignore: mypy thinks the RHS is None; we need to fix the annotation for
# @cached here.
Copy link
Member

@clokep clokep Feb 9, 2023

Choose a reason for hiding this comment

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

This sounds like the mypy plugin for cached isn't working at all then? This makes me very nervous to ignore TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works okay in Synapse proper, when @cached decorates an async def and is awaited. I don't think it works well if you're trying to get at the underlying Deferred machinery, as we are here.

Copy link
Member

Choose a reason for hiding this comment

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

Does it Just Work™️ if we use async def instead?

@DMRobertson DMRobertson self-assigned this Feb 16, 2023
@DMRobertson
Copy link
Contributor Author

Haven't got the time to look at this right now. Going to close it for now since we weren't very happy with how the mypy plugin handles @cached wrapping a function that returns a deferred

@DMRobertson
Copy link
Contributor Author

Superseded by #15659 and #15658

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.

2 participants