Skip to content

Fix Any subclassing in SQLAlchemy #9492

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 5 commits into from
Jan 11, 2023
Merged

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Jan 11, 2023

Fixes 10 Any subclassing issues:

  • The same 3 in SQLAlchemy, Flask-Migrate and Flask-SQLAlchemy
  • Removed _AsyncIoGreenlet wich, looking at source code and from the name of the class and module, seems to be implementation details

Ref: #9491, #9493

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 11, 2023

The sqlalchemy-stubs package is an alternative to this package and also includes a mypy plugin for more precise types.

How much of an "alternative" is types-SQLAlchemy to sqlalchemy-stubs? Is the latter too reliant on mypy-plugin? Or is the former only there so we could reference it in Flask-SQLAlchemy (which itself is referenced in Flask-Migrate) ?
Because if we can just drop both SQLAlchemy (in favor of sqlalchemy-stubs/sqlalchemy2-stubs) and Flask-Migrate (already planned), it would be a lot easier.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 11, 2023

Hmm, I'd prefer us to find another solution here :/

It feels really weird for us to declare a dependency on mypy so that we can provide accurate stubs for a directory that only exists to provide a mypy plugin for a completely separate set of SQLAlchemy stubs.

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 11, 2023

Come to think of it, weird that mypy_test can't find mypy. We shouldn't need to depend on it at all. I guess it's because of the virtual environment being created.

This should be better now :)

@Avasam Avasam changed the title Use mypy type in SQLAlchemy/ext/mypy Fix Any subclassing in SQLAlchemy Jan 11, 2023
@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 11, 2023

Just as a discussion point. Since SQLAlchemy 2.0 is typed. And sqlalchemy2-stubs is actively being developed for 1.4 by the same organization (replacing dropbox's sqlalchemy-stub). And we can now reference external stubs.
Does typeshed plan on continuing to maintain types-SQLAlchemy once those come out of pre-release?

@srittau
Copy link
Collaborator

srittau commented Jan 11, 2023

sqlalchemy2-stubs has some shortcomings, mostly around the fact that it requires a mypy plugin. I had some more plans with our stubs, but we will most likely drop the stubs some point after SQLAlchemy 2 comes out, whenever that is. That said, I'm opposed to depend on sqlalchemy2-stubs.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 11, 2023

@srittau, do you think we actually need the ext/mypy directory? It feels weird having stubs for a mypy plugin that's only there at runtime in order to support another set of stubs. Could we just delete the directory?

@srittau
Copy link
Collaborator

srittau commented Jan 11, 2023

@AlexWaygood Sure. I think we have too many internal modules in our stubs anyway.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you!

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 11, 2023

@AlexWaygood Sure. I think we have too many internal modules in our stubs anyway.

Gotta love fixing things by just removing code !

@AlexWaygood
Copy link
Member

@AlexWaygood Sure. I think we have too many internal modules in our stubs anyway.

Gotta love fixing things by just removing code !

Fixed a bug by removing the feature ;)

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit 5f08529 into python:main Jan 11, 2023
@Avasam Avasam deleted the mypy-in-SQLAlchemy branch January 11, 2023 11:37
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.

3 participants