-
Notifications
You must be signed in to change notification settings - Fork 192
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
Dependencies: Update to sqlalchemy~=2.0
#6146
Conversation
92656b5
to
2386b18
Compare
2386b18
to
9cc31a9
Compare
9cc31a9
to
9fe318f
Compare
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.
A few general questions:
- What's with all the
mypy
/pylint
overrides? - Lots of failed tests at the bottom. Plan?
For 1, I see that you also updated the sqlalchemy[mypy]
pre-commit dependency? Related?
Thanks for the review @edan-bainglass
Typing changed a lot in v2.0 (see below). There are some issues where these lead to false positives: sqlalchemy/sqlalchemy#7726
Do you mean the red crosses at the end of the changes in the github interface? Those are not failed tests. All tests pass (see bottom of this PR where there is a box of all checks and it says "All checks have passed"). They are just "annotations" that Github adds in scanning the code. They are quite dumb since it just does some static analysis and trips over
For v2.0, we no longer need a separate |
edba2e5
to
6489123
Compare
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.
@sphuber Thanks for answering my previous questions.
LGTM!
A number of minor changes were required for the update: * Queries that use `order_by` now need to include the property that is being ordered on in the list of projections. * The `Session.bulk_update_mappings` and `Session.bulk_insert_mappings` are replaced by using `Session.execute` with the `update` and `insert` methods. * The `sqlalchemy-utils` dependency is no longer used as well as the `tests/storage/psql_dos/test_utils.py` file that used it. * The `future=True` is removed from the engine creation. This was a temporary flag to enable v2.0 compatibility with v1.4. * Test of schema equivalence for export archives needed to be updated since the casting of `UUID` columns for PostgreSQL changed. * Remove the `sphinx-sqlalchemy` dependency since it is not compatible with `sqlalchemy~=2.0`. The documentation that relied on it to show the database models is temporarily commented out.
6489123
to
99960c6
Compare
A number of minor changes were required for the update:
order_by
now need to include the property that isbeing ordered on in the list of projections.
Session.bulk_update_mappings
andSession.bulk_insert_mappings
are replaced by using
Session.execute
with theupdate
andinsert
methods.
sqlalchemy-utils
dependency is no longer used as well as thetests/storage/psql_dos/test_utils.py
file that used it.future=True
is removed from the engine creation. This was atemporary flag to enable v2.0 compatibility with v1.4.
since the casting of
UUID
columns for PostgreSQL changed.sphinx-sqlalchemy
dependency since it is not compatiblewith
sqlalchemy~=2.0
. Temporarily comment out its usage in docs.