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

[SQLAlchemy] Annotate row classes #9568

Merged
merged 8 commits into from
Mar 15, 2023
Merged

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jan 18, 2023

No description provided.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator Author

srittau commented Jan 18, 2023

The new homeassistant warning is due the comment I left about Row.count.

https://github.com/home-assistant/core/blob/37c1052cceadec109d681ec3bced6c3f96a5863c/homeassistant/components/recorder/statistics.py#L1184

The comment:

    # The count and index methods are inherited from Sequence.
    # If the result set contains columns with the same names, these
    # fields contains their respective values, instead. We don't reflect
    # this in the stubs.

While we could of course type count and index as Any, I think an explicit cast here is actually advantageous as it's a clear sign that something "interesting" is going on here and it's not the normal Sequence.count field that is being used.

@Avasam
Copy link
Collaborator

Avasam commented Jan 19, 2023

Is this another case for a permissive union? count: AnyOf[Callable[[Any], int], int]

Maybe not if it's not that common. If a user knows they's using count and index column
(or any other column name that overrides an existing property) they could possibly subtype Row and explicitly set the type for their class. Or just use a cast.

@github-actions

This comment has been minimized.

stubs/SQLAlchemy/sqlalchemy/cresultproxy.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/engine/row.pyi Outdated Show resolved Hide resolved
stubs/SQLAlchemy/sqlalchemy/cresultproxy.pyi Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

@srittau could you respond to Avasam's feedback?

@github-actions

This comment has been minimized.

Co-authored-by: Avasam <samuel.06@hotmail.com>
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

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

@srittau
Copy link
Collaborator Author

srittau commented Mar 9, 2023

Whoops, I forgot about this PR. Thanks, @Avasam for the thorough PR. I think I addressed all the concerns, although I fixed things a bit differently.

@JelleZijlstra JelleZijlstra merged commit a544b75 into python:main Mar 15, 2023
@srittau srittau deleted the sqlalchemy-row branch March 15, 2023 10:43
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