Skip to content
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 for SQLAlchemy 2.0 #249

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 30 additions & 25 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,37 +10,42 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: ["3.7", "3.9", "3.11"]
python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"]
sqlalchemy-version: ["1.4", "2.0"]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: pyproject.toml
- name: Install dependencies
run: pip install -e .[test] coveralls
- name: Test with pytest
run: coverage run -m pytest
- name: Upload Coverage
run: coveralls --service=github
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
COVERALLS_FLAG_NAME: ${{ matrix.test-name }}
COVERALLS_PARALLEL: true
SQLALCHEMY_WARN_20: "true"
- uses: actions/checkout@v3
Copy link
Owner

Choose a reason for hiding this comment

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

I only just noticed: what caused the reformatting? That makes it hard to see the actual changes.

Copy link
Owner

Choose a reason for hiding this comment

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

If you could undo the formatting changes and only leave the actual changes in, then I'm ready to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting removed in #275

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: pyproject.toml
- name: Install dependencies SQLAlchemy 1.4
if: matrix.sqlalchemy-version == 1.4
run: pip install -e .[test,sqlmodel] coveralls SQLAlchemy==1.4.*
- name: Install dependencies SQLAlchemy 2.0
if: matrix.sqlalchemy-version == 2.0
run: pip install -e .[test] coveralls SQLAlchemy==2.0.*
- name: Test with pytest
run: coverage run -m pytest
- name: Upload Coverage
run: coveralls --service=github
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
COVERALLS_FLAG_NAME: ${{ matrix.test-name }}
COVERALLS_PARALLEL: true
SQLALCHEMY_WARN_20: "true"

coveralls:
name: Finish Coveralls
needs: [test]
runs-on: ubuntu-latest
container: python:3-slim
steps:
- name: Finished
run: |
pip install coveralls
coveralls --service=github --finish
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- name: Finished
run: |
pip install coveralls
coveralls --service=github --finish
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
10 changes: 7 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ classifiers = [
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
]
requires-python = ">=3.7"
dependencies = [
"SQLAlchemy >= 1.4.36, < 2.0",
"SQLAlchemy >= 1.4.36",
"inflect >= 4.0.0",
"importlib_metadata; python_version < '3.10'",
]
Expand All @@ -44,6 +45,8 @@ test = [
"pytest-cov",
"psycopg2-binary",
"mysql-connector-python",
]
Comment on lines 47 to +48
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"mysql-connector-python",
]
"mysql-connector-python",
"sqlacodegen[sqlmodel]",
]

Copy link
Owner

Choose a reason for hiding this comment

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

This will cause the sqlmodel tests to actually run, and restore the lost coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Oh right. Hmm, those tests need to be run somehow.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you had the right idea, but couldn't you just have compared the sqlalchemy version to the string 1.4.* and conditionally installed sqlmodel then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many roads that lead to Rome. This way has the advantage that if one wants to pin SQLalchemy to a certain version only the install line needs to be changed.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not going to want to pin it to a specific version.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems resolved to me since bbfacf7, see lines 24-29

sqlmodel = [
"sqlmodel",
]
citext = ["sqlalchemy-citext >= 1.7.0"]
Expand Down Expand Up @@ -75,7 +78,8 @@ strict = true
plugins = ["sqlalchemy.ext.mypy.plugin"]

[tool.pytest.ini_options]
addopts = "-rsx --tb=short"
pythonpath = "src/sqlacodegen"
Copy link
Owner

Choose a reason for hiding this comment

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

This is unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in #275

addopts = "-rsx --tb=short --import-mode=importlib"
Copy link
Owner

Choose a reason for hiding this comment

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

Why --import-mode=importlib?

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in #275

Copy link
Contributor Author

Choose a reason for hiding this comment

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

testpaths = ["tests"]

[coverage.run]
Expand All @@ -88,7 +92,7 @@ show_missing = true
[tool.tox]
legacy_tox_ini = """
[tox]
envlist = py37, py38, py39, py310
envlist = py37, py38, py39, py310, py311
skip_missing_interpreters = true
isolated_build = true

Expand Down
Loading