Skip to content

TYP:Replace union of subclasses with base class. #49587

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

Merged
merged 1 commit into from
Nov 14, 2022
Merged

TYP:Replace union of subclasses with base class. #49587

merged 1 commit into from
Nov 14, 2022

Conversation

cdcadman
Copy link
Contributor

@cdcadman cdcadman commented Nov 8, 2022

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

I am splitting this out of #49531 . In order to refactor the code in that PR, I had to change the return type of pandasSQL_builder, and that forced me to make additional changes in order to make mypy pass in CI:

  • Line 581 (pandas_sql.meta.reflect(bind=pandas_sql.connectable, only=[sql])) was failing mypy, but it can be deleted since the table is reflected in SQLDatabase.get_table, when the sqlalchemy.Table object is created.
  • I declared all required functions in PandasSQL and made the abstract. I then made all the arguments agree between PandasSQL and subclasses.

@mroeschke mroeschke added IO SQL to_sql, read_sql, read_sql_query Typing type annotations, mypy/pyright type checking labels Nov 9, 2022
@cdcadman
Copy link
Contributor Author

@mroeschke , is it ok to merge the PR's that I create out of #48576 one at a time, or do you want me to get all the PR's set up and ready to merge first?

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM (and okay to merge before the other PRs are ready) @twoertwein if you have any other comments

@mroeschke mroeschke added this to the 2.0 milestone Nov 14, 2022
@twoertwein twoertwein merged commit 4dacf56 into pandas-dev:main Nov 14, 2022
@twoertwein
Copy link
Member

Thank you @cdcadman !

@cdcadman cdcadman deleted the pandassql branch November 15, 2022 03:33
MarcoGorelli pushed a commit to MarcoGorelli/pandas that referenced this pull request Nov 18, 2022
mliu08 pushed a commit to mliu08/pandas that referenced this pull request Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO SQL to_sql, read_sql, read_sql_query Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants