db: adapt SopelDB to SQLAlchemy 2.x new style #2243
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgw
reviewed
Feb 23, 2022
|
Yeah, this one's good for a last round of review! |
dgw
approved these changes
May 13, 2022
ghost
reviewed
May 13, 2022
SQLAlchemy 1.4 (used by Sopel) defines a cross-compatible new style to query the database using its ORM, its engine, and its connection: * Engine cannot execute SQL directly * Connection can be used as context manager * Session can be used as context manager * Instead of doing *.query, use *.execute(stmt) * SQL statements are easier to create with select, update, and delete function from `sqlalchemy.sql` * And many other things One of the key change here is the usage of session object as context manager instead of using try/except block: less code, same feature! We can't set `future=True` on the Engine yet, because we need the non-compatible and now deprecated behavior of direct SQL execution from the Engine object: `SopelDB.execute` has been deprecated and should be removed in Sopel 8.1, so we can make Sopel compatible with both SQLAlchemy 1.4 and 2.0, as planned by SQLAlchemy migration strategy. In order to lower dgw's blood pressure, there are new tests, existing tests have been updated as well, and the coverage has increased from ~70% to ~85%. Also, more doc, because I always write more doc. Co-authored-by: dgw <dgw@technobabbl.es>
As noted by dgw, it's better to replace Sopel's owner/dev/users nicks by more neutral references to Monty Python characters and actors. Is that a dead parrot?
Exirel
force-pushed
the
db-for-sqlalchemy20
branch
from
e25a4b1
to
d4189c2
Compare
|
Squashed! |
|
*stares in Klingon at @RustyBower* (not really to rush you, but to give a status update in-band since we've only talked about your ongoing review on IRC) |
RustyBower
requested changes
Jun 2, 2022
|
@dgw all good now! |
|
I think you meant to ping @RustyBower for final thumbs-up, lol. This already got a ✔️ from me. 😁 |
|
Hm, I'm not sure we are going to get that review from @RustyBower. I'm pretty confident in my change in any case. |
|
Still LGTM :) |
Well the PR says "@RustyBower requested changes" so... 😁 |
RustyBower
approved these changes
Jul 11, 2022
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
SQLAlchemy 1.4 (used by Sopel) defines a cross-compatible new style to query the database using its ORM, its engine, and its connection:
sqlalchemy.sqlOne of the key change here is the usage of session object as context manager instead of using try/except block: less code, same feature!
We can't set
future=Trueon the Engine yet, because we need the non-compatible and now deprecated behavior of direct SQL execution from the Engine object:SopelDB.executehas been deprecated and should be removed in Sopel 8.1, so we can make Sopel compatible with both SQLAlchemy 1.4 and 2.0, as planned by SQLAlchemy migration strategy.In order to lower @dgw 's blood pressure, there are new tests, existing tests have been updated as well, and the coverage has increased from ~70% to ~85%.
Also, more doc, because I always write more doc.
PS: yes, some tests have been moved around to follow the same order of declaration as the method they are testing.
Checklist
make qa(runsmake qualityandmake test)