🏷️ Adjust type annotation for Field.sa_type to support instantiated SQLAlchemy types

[diachkow]

Note

When I was writing description for this PR, I found another discussion started for just the same issue I was experiencing with mypy, so this changes are basically fixing the issue described here

Using sa_type and sa_column_kwargs instead of just sa_column can benefit when using inheritance for classes derived from SQLModel as it was suggested here.

If sa_column is not specified and sa_type is provided, it will be passed as a second, type argumnet to sqlalchemy.Column instance. If you would check sqlalchemy.Column construction definition, it looks as following:

def__init__( self, __name_pos: Optional[ Union[str, _TypeEngineArgument[_T], SchemaEventTarget] ] =None, __type_pos: Optional[ Union[_TypeEngineArgument[_T], SchemaEventTarget] ] =None, *args: SchemaEventTarget, name: Optional[str] =None, type_: Optional[_TypeEngineArgument[_T]] =None, autoincrement: _AutoIncrementType="auto", default: Optional[Any] =_NoArg.NO_ARG, insert_default: Optional[Any] =_NoArg.NO_ARG, doc: Optional[str] =None, key: Optional[str] =None, index: Optional[bool] =None, unique: Optional[bool] =None, info: Optional[_InfoType] =None, nullable: Optional[ Union[bool, Literal[SchemaConst.NULL_UNSPECIFIED]] ] =SchemaConst.NULL_UNSPECIFIED, onupdate: Optional[Any] =None, primary_key: bool=False, server_default: Optional[_ServerDefaultArgument] =None, server_onupdate: Optional[_ServerOnUpdateArgument] =None, quote: Optional[bool] =None, system: bool=False, comment: Optional[str] =None, insert_sentinel: bool=False, _omit_from_statements: bool=False, _proxies: Optional[Any] =None, **dialect_kwargs: Any, ):

Note the __type_pos argument with Union[_TypeEngineArgument[_T], SchemaEventTarget] where

_TypeEngineArgument=Union[Type["TypeEngine[_T]"], "TypeEngine[_T]"]

So, from technical perspective you can pass not only the subclass of TypeEngine, e.g. SQLAlchemy's sqltype such as String, Integer, DateTime, JSON etc, but also an instance of this type.

I was trying for JSONB(none_as_null=True) and String(50) and it worked just fine, alembic migrations were generated correctly, only mypy was arguing for type mismatch with call-overload issue.

To fix mypy error, we can update type annotation for sqlmodel.main.Field.sa_type to support also an instantiated SQLAlchemy's sqltype to match those of sqlalchemy.Column

Related discussions:

• Typing errors when using `sa_type` and "specific" SQLAlchemy types #1228
• Bad type annotation for sa_type #1669
• Using sa_type causes Mypy type error #955

[YuriiMotov]

LGTM

Simple code example to check (in the details)

Details

fromdatetimeimportdatetimefromsqlmodelimportDateTime, Field, SQLModel, create_engineclassA(SQLModel): created_at: datetime=Field(sa_type=DateTime(timezone=False)) engine=create_engine("sqlite:///") SQLModel.metadata.create_all(engine)

Running mypy gives

error: No overload variant of "Field" matches argument type "DateTime" [call-overload] 

on master and

Success: no issues found in 1 source file 

after applying this fix

[YuriiMotov]

LGTM
See #1345 (review)

[diachkow]

Thanks for approval! Who is responsible for merging this, what are the rules in this repo?

[YuriiMotov]

Thanks for approval! Who is responsible for merging this, what are the rules in this repo?

Only Sebastian can merge it. I already forwarded it to him. We should just wait