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 postgresql+asyncpg scheme prefix (closes #395) #396

Merged
merged 22 commits into from Sep 25, 2021

Conversation

mhadam
Copy link
Contributor

@mhadam mhadam commented Sep 22, 2021

No description provided.

databases/core.py Outdated Show resolved Hide resolved
@mhadam
Copy link
Contributor Author

mhadam commented Sep 23, 2021

Ok, I bring this up only because SQLAlchemy supports the format dialect+driver and uses postgresql+asyncpg throughout documentation, and since I'm using SQLAlchemy elsewhere in a project I can't reuse that URL with Databases (since postgresql+asyncpg raises a KeyError).

More background: I'm writing a FastAPI project and using fastapi-users which requires Databases. So I'm stuck having to supply a DB URL to both libraries. Ideally these should be able to be the same, and since one's built on the other, they shouldn't act differently anyway.

If it's not ok to have multiple mappings for a backend (i.e. specifying all valid schemes), I propose the following strategy:

  1. check to see if the complete scheme is supported
  2. otherwise check if the dialect is supported, use that backend and ignore the driver if specified

@aminalaee
Copy link
Member

I agree that making it specific dialect+driver looks better. But as you said if we could handle both cases it would be great for backwards-compatibility too.
This just needs the missing test coverage, and we can merge it.

@mhadam
Copy link
Contributor Author

mhadam commented Sep 23, 2021 via email

tests/test_databases.py Outdated Show resolved Hide resolved
tests/test_databases.py Outdated Show resolved Hide resolved
@aminalaee aminalaee linked an issue Sep 24, 2021 that may be closed by this pull request
.github/workflows/test-suite.yml Outdated Show resolved Hide resolved
Co-authored-by: Amin Alaee <mohammadamin.alaee@gmail.com>
tests/test_databases.py Show resolved Hide resolved
tests/test_databases.py Show resolved Hide resolved
tests/test_databases.py Outdated Show resolved Hide resolved
tests/test_databases.py Outdated Show resolved Hide resolved
tests/test_databases.py Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

Thanks for this. I fixed the rest of tests.

@aminalaee aminalaee merged commit 3525ca5 into encode:master Sep 25, 2021
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.

Add support for postgresql+asyncpg DB scheme
2 participants