Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typing to Pool.acquire #1209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielNoord
Copy link
Contributor

Now that #1197 landed I could take the patch I suggested in #578 and add some more minor changes.

Therefore, this closes #578 and closes #521

In the long run, this will probably cause issues. Proxies can't really be expressed that well on the type level, so we will probably get a lot of "Unknown attribute" errors on everything that is using whatever comes out of acquire().
However, I would personally advise not to worry about this too much. Let's first add typing to those places where we know it will work in incremental steps and consider larger issues when we have some more experience with adding the types.

We'll likely also run into issues identified in #577 but as long as we don't add any typing that is incorrect this can still be solved.

There is (obviously) one behaviour change with the assert, but as I read the code (and tests) I don't think that should be an actual change in behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant