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

Test without sqlalchemy-citext #2518

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjmayo
Copy link
Contributor

@cjmayo cjmayo commented Sep 4, 2024

The last sqlalchemy-citext update was in 2021 and it has various compatibility issues.

SQLAlchemy now has CITEXT support.

@cjmayo cjmayo force-pushed the sqlalchemy-citext branch 3 times, most recently from 3468d1e to 6b49729 Compare September 4, 2024 18:39
@cjmayo
Copy link
Contributor Author

cjmayo commented Sep 5, 2024

Whether this is case sensitive and whether we are testing it I am unsure:

@converts('Text', 'LargeBinary', 'Binary', 'CIText') # includes UnicodeText

@cjmayo
Copy link
Contributor Author

cjmayo commented Sep 7, 2024

Added a new commit - a wider problem - doesn't look like we have been testing SQLAlchemy 1.

With use_frozen_constraints constraints are generated by calling "pip
freeze" after the environment deps have been installed and before the
package dependencies are installed, thus constraints-sqlalchemy1.txt is
being ignored.

An idea of how to fix it in the added commit. At least this was all I could find. N.B. the formatted output of the GitHub Actions can truncate the list of pip installed packages.

@cjmayo
Copy link
Contributor Author

cjmayo commented Oct 13, 2024

I'll rebase after #2540.

@cjmayo cjmayo force-pushed the sqlalchemy-citext branch 3 times, most recently from 76a1cf9 to 527daea Compare October 14, 2024 18:35
@cjmayo cjmayo marked this pull request as ready for review October 14, 2024 18:37
@cjmayo
Copy link
Contributor Author

cjmayo commented Oct 14, 2024

Rebased.

@cjmayo cjmayo force-pushed the sqlalchemy-citext branch from 527daea to 7b2dc1d Compare October 14, 2024 18:55
@cjmayo cjmayo marked this pull request as draft October 14, 2024 18:59
@cjmayo cjmayo force-pushed the sqlalchemy-citext branch 2 times, most recently from c68619a to 6c9e7c8 Compare October 15, 2024 18:26
@cjmayo cjmayo marked this pull request as ready for review October 15, 2024 18:26
@cjmayo
Copy link
Contributor Author

cjmayo commented Oct 15, 2024

Finished experimenting.

@cjmayo cjmayo mentioned this pull request Oct 16, 2024
@samuelhwilliams
Copy link
Contributor

This looks sensible. Would you mind adding an entry in doc/changelog.rst under a new v2.0.0a2 heading please?

@cjmayo
Copy link
Contributor Author

cjmayo commented Oct 27, 2024

Rebase this one? doc/changelog.rst has updated since.

The last sqlalchemy-citext update was in 2021 and it has various
compatibility issues.

SQLAlchemy now has CITEXT support.
@cjmayo
Copy link
Contributor Author

cjmayo commented Nov 24, 2024

Rebased and changelog updated.

@@ -36,7 +36,6 @@ sqlalchemy = [
sqlalchemy-with-utils = [
"Flask-Admin[sqlalchemy]",
"sqlalchemy_utils>=0.38.0",
"sqlalchemy-citext>=1.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I've had a bit more of a think about this since my last comment - sorry.

We still theoretically support sqlalchemy<2.0.19 - this feels like it would break users on SQLAlchemy v1 and I'm not sure we've made the decision to drop support for that yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks more complicated because sqlalchemy-citext has not been updated for 3 years:

mahmoudimus/sqlalchemy-citext#23

mahmoudimus/sqlalchemy-citext#25

mahmoudimus/sqlalchemy-citext#26

I don't know whether it is possible to get away with it (if using psycopg2), our simple test passes. But ultimately this is recommending something that is broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants