Add sqlalchemy test matrix to ensure compatibility with 1.3 version #254
Conversation
mdesmet
force-pushed
the
bug/sqlalchemy-matrix
branch
2 times, most recently
from
e112407 to
877f865
Compare
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
seems to be an unfortunate situation since we're adding a matrix but also removing most of the tests.
How do other clients provide support for multiple SQLAlchemy versions?
.github/workflows/ci.yml
Outdated
| sudo apt-get update | ||
| sudo apt-get install libkrb5-dev | ||
| pip install .[tests] | ||
| pip install .[tests] sqlalchemy~=${{ matrix.sqlalchemy }} |
There was a problem hiding this comment.
I'd instead say to inline the constraint in the matrix entry itself.
i.e. ~=1.4.0 (latest 1.4)
==1.4.2 (some specific version)
| @pytest.mark.skipif( | ||
| sqlalchemy_version() < "1.4", | ||
| reason="columns argument to select() must be a Python list or other iterable" | ||
| ) |
There was a problem hiding this comment.
we're basically dropping lot of coverage. Seems like to make sure we can reasonably rely on the matrix we'd need to create two test classes or modify tests to work with both.
There was a problem hiding this comment.
Agreed but I suggest we do that as a follow up issue. Most test cases are basically sqlalchemy feature tests. In this fix we mainly lay the foundation (the test matrix) and fix the issue at hand: #250
| ), | ||
| ), | ||
| ], | ||
| ] if sqlalchemy_version() >= "1.4" else [], # make_url is not supported in sqlalchemy 1.3 |
There was a problem hiding this comment.
do docs need updating as well that the URL static helper can only be used with 1.4.x?
There was a problem hiding this comment.
Actually I fixed it now by adding a slash after the port. So now the helper is working for 1.3 and 1.4.
mdesmet
force-pushed
the
bug/sqlalchemy-matrix
branch
from
877f865 to
b48d1e1
Compare
|
Let's create a compatibility section under https://github.com/trinodb/trino-python-client#sqlalchemy and document which SQLAlchemy versions are supported. |
mdesmet
force-pushed
the
bug/sqlalchemy-matrix
branch
from
b48d1e1 to
9ed85b1
Compare
Let's handle this as follow up. |
|
@mdesmet what's wrong with simply adding a dependency constraint that only SQLAlchemy >= 1.4 is supported? This adheres to the KISS principle and doesn't pollute the code, unit tests, CI build matrix etc. |
|
Because we can't control what other third party tools use as the version of SQLAlchemy. I'd love to be able to force a version of SQLAlchemy on the consumer to simply our lives but it's not possible today. |
4 participants
Fixes #250
Description
scalar_one()andscalar_one_or_none()are not available in sqlalchemy 1.3. They have been replaced byscalar()calls.sqlalchemy has also been added into the ci matrix to ensure compatibility with 1.3 and 1.4 versions.
Non-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:
* Use sqlalchemy 1.3 compatible methods in sqlalchemy dialect.