Skip to content

Conversation

fike
Copy link
Contributor

@fike fike commented Mar 23, 2021

Change Summary

SQLAchemy 1.4 added alpha support to async calls. The support to PostgreSQL needs to add asyncpg in the schema, e.g.: postgresql+asyncpg://scott:tiger@localhost/test

Related issue number

#2545
Closes #2679

Checklist

  • [ X ] Unit tests for the changes exist
  • [ X ] Tests pass on CI and coverage remains at 100%
  • [ X ] Documentation reflects the changes where applicable
  • [ X ] changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #2567 (13b9237) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 13b9237 differs from pull request most recent head d0a5f8d. Consider uploading reports for the commit d0a5f8d to get more accurate results

@@            Coverage Diff            @@
##            master     #2567   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5109           
  Branches      1050      1050           
=========================================
  Hits          5109      5109           
Impacted Files Coverage Δ
pydantic/networks.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5549e8d...d0a5f8d. Read the comment docs.

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Thanks!

@Kludex
Copy link
Member

Kludex commented Mar 28, 2021

As we are already here... Can we add pg8000 as well? 😅

@fike
Copy link
Contributor Author

fike commented Mar 29, 2021

As we are already here... Can we add pg8000 as well? 😅

Certainly! I'm changing of the computer, if can waiting for a few days until finish my setup and implement it. :D

@fike
Copy link
Contributor Author

fike commented Mar 31, 2021

As we are already here... Can we add pg8000 as well? 😅

Hi @Kludex, I added pg8000 here. :)

Co-authored-by: Stephen Brown II <Stephen.Brown2@gmail.com>
Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

You can probably rebase on master too

@PrettyWood PrettyWood added awaiting author revision awaiting changes from the PR author and removed ready for review labels Apr 5, 2021
@fike fike requested a review from PrettyWood April 6, 2021 02:25
@fike
Copy link
Contributor Author

fike commented Apr 6, 2021

You can probably rebase on master too

Hi @PrettyWood, can you check if I did right? I'm not confident that if my laptop migration didn't mess something.

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Just a suggestion for the change file to use ` and present. LGTM otherwise for the patch :)

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
@fike
Copy link
Contributor Author

fike commented Apr 7, 2021

Just a suggestion for the change file to use ` and present. LGTM otherwise for the patch :)

Thanks! :)

@PrettyWood
Copy link
Collaborator

Hey @fike
Seeing #2679, I wonder if we could just add all the dialects specified here
WDYT ?

@fike
Copy link
Contributor Author

fike commented Apr 20, 2021

Hey @fike
Seeing #2679, I wonder if we could just add all the dialects specified here
WDYT ?

Yes, certainly! I'll do that. 👍

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Thanks!

fike and others added 4 commits April 22, 2021 08:57
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

@samuelcolvin
Copy link
Member

please update.

@github-actions github-actions bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels May 9, 2021
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@fike
Copy link
Contributor Author

fike commented May 9, 2021

please update.

I did. Thanks, @samuelcolvin. :)

Comment on lines 22 to 26
'postgresql+asyncpg://user:pass@localhost:5432/app',
'postgresql+pg8000://user:pass@localhost:5432/app',
'postgresql+psycopg2cffi://user:pass@localhost:5432/app',
'postgresql+py-postgresql://user:pass@localhost:5432/app',
'postgresql+pygresql://user:pass@localhost:5432/app',
Copy link
Collaborator

Choose a reason for hiding this comment

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

All those tests would already be valid without your PR. You probably want to parametrize test_postgres_dsns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that but I'm sure if my implementation was good. It's wonderful anyone can review it. :)

@PrettyWood
Copy link
Collaborator

Please update and rebase/merge with master for CI to run properly. Sorry for reaching out so late. Thanks!

@fike
Copy link
Contributor Author

fike commented Jul 25, 2021

@PrettyWood, can you check if I rebased correctly? :)

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Thanks!

@PrettyWood PrettyWood merged commit 1a9f59d into pydantic:master Sep 3, 2021
@PrettyWood PrettyWood removed the awaiting author revision awaiting changes from the PR author label Sep 3, 2021
@Kludex Kludex mentioned this pull request Sep 4, 2021
4 tasks
jpribyl pushed a commit to liquet-ai/pydantic that referenced this pull request Oct 7, 2021
* Added suport to postgresql async driver

* Added postgres+asyncpg in the doc.

* Added changes file

* Added postgresql+pg8000 schema

* ran make format

* Update docs/usage/types.md

Co-authored-by: Stephen Brown II <Stephen.Brown2@gmail.com>

* Changed from schema to scheme

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* fixed typo scheme

* fixed merge schema

* changed to one line description

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* added others DBAPI dialects.

* Added two # by PrettyWood

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* fix typo by PrettyWood

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* Fixed typo by PrettyWood

Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>

* added postgresql+psycopg2 in changes dir

* Fixed typo by @samuelcolvin

Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

* docs: fix typo

* chore: sort alphabetically

Co-authored-by: Fernando Ike <fernando.ike@maburix.com>
Co-authored-by: Stephen Brown II <Stephen.Brown2@gmail.com>
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
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.

Can't use postgresql+psycopg2://... with PostgresDsn
5 participants