-
-
Notifications
You must be signed in to change notification settings - Fork 20
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 support to SQLAlchemy 2.x #97
Add support to SQLAlchemy 2.x #97
Conversation
Here are the changes applied to support the tSQLAlchemy >=2.x: * black and flake8 check * upgrade black and flake8 * update call for the SQLAlchemy 2.x * upgrade dependencies to support SQLAlchemy 2.x and fix call.
Hi @siddhantgoel |
foo_count = ( | ||
db.get_engine('foo') | ||
.execute("SELECT COUNT(*) FROM foo") | ||
.fetchone()[0] | ||
) | ||
bar_count = ( | ||
db.get_engine('bar') | ||
.execute("SELECT COUNT(*) FROM bar") | ||
.fetchone()[0] | ||
) | ||
with db.get_engine('foo').begin() as conn: | ||
foo_count = conn.execute( | ||
text("SELECT COUNT(*) FROM foo") | ||
).fetchone()[0] | ||
|
||
with db.get_engine('bar').begin() as conn: | ||
bar_count = conn.execute( | ||
text("SELECT COUNT(*) FROM bar") | ||
).fetchone()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for me to understand this correctly, if we don't introduce this change in the test, then the test would fail in case SQLAlchemy 1.x is installed? Or is this how the call is made in the latest 1.x release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the call should be written in 2.x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in that case we'll need to write some compatibility code inside the main library. Otherwise we'll be breaking usage for 1.x users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'm checking the https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#first-prerequisite-step-two-a-working-1-4-application to identify changes. Let me test it with the last 1.4 version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for looking it up. In that case we should probably specify something like SQLAlchemy >= 1.4
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added on 109c2ab
Co-authored-by: Siddhant Goel <[email protected]>
Hi @siddhantgoel |
Hey, thanks for pushing the changes. I left one comment and the CI is complaining about something that we should fix before we can merge it in! |
SQLAlchemy >= 1.4 and Python 3.8.1 for Flake8.
I just published v0.8.0 which should include this fix. The only change I had to make was to adjust the dependency string slightly (replace |
Awesome, thank you very much! |
Summary
This PR upgrade packages using the dependency resolution from
poetry
.No changes were applied to the package code.
Tests were reviewed and adjusted to be compatible with the SQLAlchemy 2.x.
Tests are passing:
Here are the changes applied to support the tSQLAlchemy >=2.x:
Previous PRs logged: #95 and #96