bryanforbes / asyncpg-stubs

BSD 3-Clause "New" or "Revised" License
37 stars 4 forks source link

Should `await asyncpg.create_pool()` return `Pool` instead of `None | Pool` #120

Open sbdchd opened 2 years ago

sbdchd commented 2 years ago

Thank you for the library!

After setting this up I found that my connection pool logic is being flagged

essentially I have:

connection_pool: asyncpg.Pool[asyncpg.Record]

def on_startup() -> None:
    global connection_pool
    connection_pool = await asyncpg.create_pool()

and I get an error:

Incompatible types in assignment (expression has type "Optional[Pool[Record]]", variable has type "Pool[Record]")  [assignment] mypy(error)

looking at Asyncpg's pool:

https://github.com/MagicStack/asyncpg/blob/40b16ea65f8b634a392e7e5b6509ee7dda45c4cd/asyncpg/pool.py#L978-L979

I think technically it can return None but it seems unlikely to happen for most use cases (maybe?)

so while Pool | None represents a possible case, I think it would be okay if it didn't include it in the types for better ergonomics

What do you think?

bryanforbes commented 2 years ago

I think you're right that await asyncpg.create_pool() will never produce None since create_pool() is returning a brand new Pool that won't have _initialize set, so awaiting the new Pool will produce itself. I imagine the best way to type this is to make create_pool() an async function so the typing for Pool.__await__ can still be correct. Does that sound like a good solution to you?

bryanforbes commented 2 years ago

By way of example, if create_pool() is typed as an async function, the following is the result:

import asyncpg

async def main() -> None:
    pool = await asyncpg.create_pool()
    reveal_type(pool)  # Pool[Record]
    reveal_type(await pool)  # Pool[Record] | None
bryanforbes commented 2 years ago

I changed the typing of asyncpg.create_pool() to an async function in e387d77f6af463e8cebe3e4601ed44adfb9cc6d0 and made a new release (0.26.5)

bryanforbes commented 2 years ago

Evidently, I didn't take into account using asyncpg.Pool as a context manager, so I'm back to the drawing board on this. I reverted the change in 94eaa963efc612018e8e89f08a7366d8e950d9f4 and released 0.26.6. This may come down to being an issue that has to be # type: ignore because the stubs are limited in what they can do. For now, you can pin to 0.26.5 (even though you won't be able to use create_pool() as a context manager), but I'll continue to think of a good solution for this problem.