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

Engine AC doesn't contain start/close_connection_pool() method. #1

Open
gmos opened this issue Nov 29, 2021 · 1 comment
Open

Engine AC doesn't contain start/close_connection_pool() method. #1

gmos opened this issue Nov 29, 2021 · 1 comment

Comments

@gmos
Copy link

gmos commented Nov 29, 2021

@app.on_event("startup")
async def open_database_connection_pool():
    engine = engine_finder()
    await engine.start_connection_pool()


@app.on_event("shutdown")
async def close_database_connection_pool():
    engine = engine_finder()
    await engine.close_connection_pool()

This code is in many of the sample applications. The engine_finder() is typed as Engine, which is an abstract class. It doesn't however provide an (@abstract)method for start_connection_pool() and close_connection_pool(). These methods only exist in the concrete subclass for Postgres. Hence vscode/pylance complain about non-existing methods.

I can do a PR, but am unsure about best solution.
Personally I would add these methods as normal methods to the Engine class, just providing a colored_warning() that pooling is not implemented for the provided engine.

Alternatively all sample programs using the connection pool could be changed like:

@app.on_event("startup")
async def open_database_connection_pool():
    engine = engine_finder()
    if isinstance(engine, PostgresEngine):
        await engine.start_connection_pool()
@dantownsend
Copy link
Member

@gmos You're right, this could be improved.

Adding a method to Engine is a good idea. As you say, it can just output a warning, and should be overridden in child classes.

Even with that change, Pylance might still complain because engine_finder can potentially return None. So the example code should be like you suggested:

@app.on_event("startup")
async def open_database_connection_pool():
    engine = engine_finder()
    if isinstance(engine, PostgresEngine):
        await engine.start_connection_pool()

If you're happy to do a PR, that would be great.

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

No branches or pull requests

2 participants