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

upgrade SQLAlchemy and pyright #687

Merged
merged 1 commit into from
May 8, 2023
Merged

Conversation

Dr-Irv
Copy link
Collaborator

@Dr-Irv Dr-Irv commented May 7, 2023

Based on pandas-dev/pandas#50558 (comment), we could upgrade SqlAlchemy to 2.0. Doing that revealed an issue in those tests and some of the typing.

Also bumped pyright to 1.1.306, but the default value for useLibraryCodeForTypes changed in that version, so added that in pyproject.toml. Without that, some of the plotting tests fail because matplotlib doesn't have typing yet.

@Dr-Irv Dr-Irv requested a review from twoertwein May 7, 2023 18:11
@@ -1095,6 +1095,7 @@ def test_read_sql_via_sqlalchemy_connection():
assert_type(read_sql("select * from test", con=conn), DataFrame),
DataFrame,
)
engine.dispose()
Copy link
Member

Choose a reason for hiding this comment

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

Can engine be used as a context manager? Might be slightly nicer if that's possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it is not possible.

| sqlalchemy.sql.expression.Selectable
| sqlalchemy.sql.expression.TextClause
| sqlalchemy.sql.Select
| FromStatement
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add those libraries (or their stubs) as a dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. I just added the two new types that seem to be valid for the test we had. We have sqlalchemy in our dev environment, but we don't want to make it something you have to install to use the stubs.

@twoertwein twoertwein merged commit 7a8befd into pandas-dev:main May 8, 2023
13 checks passed
@twoertwein
Copy link
Member

Thanks @Dr-Irv !

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

Successfully merging this pull request may close these issues.

None yet

2 participants