Skip to content

added support for sqlalchemy2 - #120

Merged
samuelcolvin merged 9 commits into
samuelcolvin:mainfrom
the-vty:main
Feb 6, 2023
Merged

added support for sqlalchemy2#120
samuelcolvin merged 9 commits into
samuelcolvin:mainfrom
the-vty:main

Conversation

@the-vty

@the-vty the-vty commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

Hi Samuel,

I noticed that the debug stopped working on sqlalchemy 2.0. Here is the PR that solves it for me.

Thanks!

@the-vty the-vty closed this Feb 1, 2023
@samuelcolvin

samuelcolvin commented Feb 1, 2023

Copy link
Copy Markdown
Owner

did you mean to close this?

@the-vty

the-vty commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author

I closed this because the tests were broken and I have no idea at the moment how to arrange the tests against the different sqlalchemy versions. Sorry for that.

@samuelcolvin

samuelcolvin commented Feb 1, 2023

Copy link
Copy Markdown
Owner

Then best to leave the PR open and we can work together to fix tests.

Looks to me like a warning from usage of declarative_base in a test, we can either ignore that warning, or use whatever sqlalchemy recommends instead of declarative_base.

@samuelcolvin samuelcolvin reopened this Feb 1, 2023
@codecov

codecov Bot commented Feb 1, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #120 (234b029) into main (8811bfe) will decrease coverage by 0.75%.
The diff coverage is 75.00%.

Additional details and impacted files 
@@             Coverage Diff             @@
##              main     #120      +/-   ##
===========================================
- Coverage   100.00%   99.25%   -0.75%     
===========================================
  Files            4        7       +3     
  Lines          279      537     +258     
  Branches        38       75      +37     
===========================================
+ Hits           279      533     +254     
- Misses           0        2       +2     
- Partials         0        2       +2
Impacted Files Coverage Δ
devtools/prettier.py 98.95% <75.00%> (ø)
devtools/utils.py 96.72% <75.00%> (ø)
devtools/__init__.py 100.00% <0.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 8811bfe...234b029. Read the comment docs.

@the-vty

the-vty commented Feb 1, 2023

Copy link
Copy Markdown
Contributor Author

I added support for models defined using declarative_base in sqlalchemy 2.0 as well.

samuelcolvin
samuelcolvin reviewed Feb 1, 2023
View reviewed changes

@samuelcolvin samuelcolvin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Otherwise LGTM

Comment thread devtools/utils.py Outdated
samuelcolvin
samuelcolvin reviewed Feb 3, 2023
View reviewed changes

@samuelcolvin samuelcolvin left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm happy with the result, but I think there's a more elegant way to do this.

Comment thread devtools/utils.py
@samuelcolvin
samuelcolvin merged commit abed0a5 into samuelcolvin:main Feb 6, 2023
@samuelcolvin

samuelcolvin commented Feb 6, 2023

Copy link
Copy Markdown
Owner

great, thanks so much.

@the-vty

the-vty commented Feb 6, 2023 via email

Copy link
Copy Markdown
Contributor Author

@tcompa tcompa mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@the-vty @samuelcolvin