Upgrade biblib to Python 3.12 - #189
Conversation
|
Coverage is 93.629% — upgrade/python-3.12 into master. No base build found for |
feeae19 to
5591744
Compare
f2c19a2 to
2f41ee8
Compare
tjacovich
left a comment
tjacovich
left a comment
There was a problem hiding this comment.
Looks like a solid upgrade. Just a couple of questions about specific implementation details.
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install "pip==24" setuptools==57.5.0 wheel | ||
| python -m pip install "pip==24" "setuptools>=62.0.0,<70.0.0" wheel |
There was a problem hiding this comment.
Out of curiosity, what packages break with setuptools >70?
There was a problem hiding this comment.
I ran some tests, and the <70 is no longer necessary, so I removed it.
| from biblib.manage import DeleteObsoleteVersionsNumber, DeleteStaleUsers, DeleteObsoleteVersionsTime | ||
| from biblib.models import User, Library, Permissions, Notes | ||
| from sqlalchemy.orm.exc import NoResultFound | ||
| from sqlalchemy import text |
There was a problem hiding this comment.
Is this strictly input sanitization?
There was a problem hiding this comment.
Not sanitization. It's a SQLAlchemy requirement for 1.4+. https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#execute-method-more-strict-execution-options-are-more-prominent
|
|
||
| import uuid | ||
| from datetime import datetime | ||
| from datetime import datetime, timezone |
There was a problem hiding this comment.
Did something change in sqlalchemy that now requires us to explicitly strip the timezone?
There was a problem hiding this comment.
The change is in Python. datetime.utcnow() is deprecated and should be replaced with datetime.now(timezone.utc), but the former is timezone-naive, and the latter is timezone-aware. I'm stripping the timezone so the new version is the same as the previous one.
There was a problem hiding this comment.
This is an interesting way to solve the relocated config. We can talk more about it at our next weekly and decide if this is how we want to do it going forward?
There was a problem hiding this comment.
That bypassed my review. I just moved the config to the root so it can be found after the changes to adsmutils.
2a6db28 to
fdd04a0
Compare
fdd04a0 to
1f3ee68
Compare
3 participants
These changes are in dev right now.