Skip to content

refactor: add type hints to flask_sqlalchemy.model #8389

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 4 commits into from
Jul 31, 2022
Merged

refactor: add type hints to flask_sqlalchemy.model #8389

merged 4 commits into from
Jul 31, 2022

Conversation

kkirsche
Copy link
Contributor

The metaclasses are typed to match the following type.__init__ overload:
https://github.com/python/typeshed/blob/master/stdlib/builtins.pyi#L167

As query's are generic in SQLAlchemy, this begins to make Model generic for use in the flask_sqlalchemy.__init__ file once this is completed.

This also updates __table_cls__ from NameMetaMixin. This returns a table from:

  1. https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/model.py#L78
  2. https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/model.py#L86
  3. https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/model.py#L94

and then has a None return if this branch hits:
https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/model.py#L90-L92

and the code runs to completion.

@kkirsche
Copy link
Contributor Author

I wasn't sure if this should be Generic[_Model] or if I should be using Query[Self] instead to indicate that it's the current model

@github-actions

This comment has been minimized.

Comment on lines 25 to 27
class Model(Generic[_Model]):
query_class: type[Query[_Model]] | None
query: Query[_Model] | None
Copy link
Member

Choose a reason for hiding this comment

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

@srittau, does making Model generic look correct to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no experience with flask-sqlalchemy, but considering that sqlalchemy.orm.query is generic: probably.

Copy link
Member

Choose a reason for hiding this comment

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

Let's YOLO it then. If it causes a problem for somebody, we'll know soon enough.

@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit ca633bb into python:master Jul 31, 2022
@kkirsche kkirsche deleted the flask-sqlalchemy-refactor branch August 1, 2022 11:45
mano7onam added a commit to mano7onam/typeshed that referenced this pull request Aug 8, 2022
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