-
-
Notifications
You must be signed in to change notification settings - Fork 141
issue 1166 Update test to use SQLAlchemy 2.0 compatible select statement #1167
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
Conversation
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.
One small change to make sure we are testing the right thing.
tests/test_io.py
Outdated
pd.read_sql( | ||
session.query(Temp.quantity).statement, session.connection() | ||
) | ||
stmt = sqlalchemy.select(Temp.quantity) |
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.
Here you are modifying the type of stmt
so you are not really testing what used to be tested. What is needed is to update the stubs so that this test passes.
Without changing this test, what needs to be done is to update a pyi file.
For that I would recommend you look into what is the new type of stmt with the update of sqlalchemy, you will see that stmt has now an extra possible type UpdateBase
and what how that impacts the stubs in pandas-stubs/io/sql.pyi
. There you see that the Type Alias _SQLStatement
is too restrictive and you need to allow UpdateBase in that TypeAlias too.
Feel free to let me know if something is unclear.
@loicdiridollou I reverted my previous commits and added extra possible type UpdateBase to _SQLStatement, is that ok? |
pandas-stubs/io/sql.pyi
Outdated
@@ -33,6 +34,7 @@ _SQLStatement: TypeAlias = ( | |||
| sqlalchemy.sql.expression.TextClause | |||
| sqlalchemy.sql.Select | |||
| FromStatement | |||
| UpdateBase |
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.
Instead of importing UpdateBase
, use the pattern as in lines 33 and 34, and place sqlalchemy.sql.expression.UpdateBase
here
@Dr-Irv Applied the change |
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.
while you were working on this, we put a constraint in pyproject.toml
on sqlalchemy
so that our CI would pass.
Can you change this line in that file:
SQLAlchemy = ">=2.0.12,<2.0.39"
to
SQLAlchemy = ">=2.0.39"
Co-authored-by: Irv Lustig <[email protected]>
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.
thanks @shirzady1934
This PR resolves #1166 by update test to use SQLAlchemy 2.0 compatible select statement