Skip to content

add type for pool.acquire() #521

New issue

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

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

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add type for pool.acquire() #521

wants to merge 3 commits into from

Conversation

Tynukua
Copy link

@Tynukua Tynukua commented Jan 8, 2020

No description provided.

@eirnym
Copy link

eirnym commented Jan 10, 2020

Looks good for me.

@@ -566,7 +566,7 @@ async def fetchrow(self, query, *args, timeout=None):
async with self.acquire() as con:
return await con.fetchrow(query, *args, timeout=timeout)

def acquire(self, *, timeout=None):
def acquire(self, *, timeout=None) -> connection.Connection:
Copy link
Contributor

Choose a reason for hiding this comment

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

This type hint only covers usual await case:

con = await pool.acquire()

But won't work properly for context manager case:

async with pool.acquire() as con:
    await con.execute(...)

The more correct return type could be something like PoolAcquireContext but we should check if mypy understands it first.

Copy link
Contributor

Choose a reason for hiding this comment

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

#577 contains more precise types

@DanielNoord
Copy link
Contributor

It is clear that this is incorrect and should be PoolAcquireContext.

I'd suggest either changing that in this PR or just closing this and wait until a PR that targets more of these simple methods is created.

@DanielNoord
Copy link
Contributor

This is superseded by #1209

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

Successfully merging this pull request may close these issues.

4 participants