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

Ana/typed mapped column #2

Closed
wants to merge 3 commits into from
Closed

Conversation

acanizares
Copy link

@acanizares acanizares commented Mar 12, 2024

NOTE: Additional changes are required before this can be merged.
Discussion: sqlalchemy#11093

Description

Summary
These are some efforts to get better typing of mapped_column. I believe the return type of mapped_column cannot be expressed correctly as it is now. It would be required to change the default value of nullable to get types correct.

Context
Right now, mapped_column returns MappedColumn[Any] so there is no guarantee that the type annotation matches the SQLA type engine passed to mapped_column (if any).
sqlalchemy#11093

When I first started the discussion I was hopeful that this could be fully solved with overloads. But now I think it is just not possible to type mapped_column correctly as long as the default value for nullable is True. Because then
there is no way to type something like mapped_column(Integer) correctly. mapped_column(Integer) can either be of type MappedColumn[int] or MappedColumn[Optional[int]], and the default should be the more specific type of those two, namely MappedColumn[int] since both of the following assignments should be valid:

x: Mapped[int]  = mapped_column(Integer)
y: Mapped[Optional[int]]  = mapped_column(Integer)

In this PR:

  1. Modified type annotations in mapped_column to
  • Return MappedColumn type matching the type engine, the nullable and the default parameters.
  1. Made TypeEngine covariant. This was necessary for assignments such as
x: Mapped[Optional[int]] = mapped_column(Integer)

to be valid for the typechecker.

  1. Added typing tests. This section gives a good overview of what this change is about.

@acanizares acanizares force-pushed the ana/typed_mapped_column branch 3 times, most recently from c7359d4 to b321b8c Compare March 12, 2024 08:45
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.

1 participant