💚 Fix linting in CI #1340
💚 Fix linting in CI #1340
Conversation
There was a problem hiding this comment.
Thank you @svlandeg! 🚀
I tweaked it a bit mainly to keep autocompletion and inline errors for final users. Although this reveals some areas that probably deserve some tweaking in the future. 🤔
I added some inline comments to share my mental process.
Thanks! 🚀
| if isinstance(model, type): | ||
| use_model = model | ||
| else: | ||
| use_model = model.__class__ | ||
| return use_model.model_fields |
There was a problem hiding this comment.
I was checking the error from Pydantic and I saw that they will deprecate accessing the model_fields from the instance in Pydantic v3, they should be accessed from the class (which makes sense).
I updated this logic here to handle that future use case. In a subsequent PR we can refactor the internal usage of this function to only pass the class and not the instance.
| @@ -477,7 +477,7 @@ def Relationship( | |||
| class SQLModelMetaclass(ModelMetaclass, DeclarativeMeta): | |||
| __sqlmodel_relationships__: Dict[str, RelationshipInfo] | |||
| model_config: SQLModelConfig | |||
| model_fields: Dict[str, FieldInfo] # type: ignore[assignment] | |||
| model_fields: ClassVar[Dict[str, FieldInfo]] | |||
There was a problem hiding this comment.
As the idea is that this should now be only in the class and not instances, I think it's good to type it right away. It won't affect at runtime if people are using it, but will start warning them in their linters, so they can start preparing for this change in Pydantic v3.
| @@ -839,7 +839,7 @@ def __tablename__(cls) -> str: | |||
| return cls.__name__.lower() | |||
|
|
|||
| @classmethod | |||
| def model_validate( | |||
| def model_validate( # type: ignore[override] | |||
There was a problem hiding this comment.
I changed this to not use kwargs and instead ignore the type here, that way people will keep having autocompletion for the params, and kwargs won't swallow extra / invald params.
Seeing this, I realize it was probably not the best idea to include the extra update here. 🤔
Also, it's probably not needed for most use cases, maybe it would make sense to remove those docs, explain how to extend data to validate an object using just dicts, then deprecate using it, and finally at some point dropping the update...
But anyway, for now, it probably works to just # type: ignore and handle it on our side so that final users get the best developer experience / editor support.
| context: Union[Dict[str, Any], None] = None, | ||
| by_alias: bool = False, | ||
| context: Union[Any, None] = None, | ||
| by_alias: Union[bool, None] = None, |
There was a problem hiding this comment.
I like to keep the signature as close to the latest Pydantic, and then do the extra work internally to support the older versions, but this way people can get used to the new syntax and/or use it if they can already upgrade to the latest version.
| @@ -896,7 +900,7 @@ def model_dump( | |||
| return super().dict( | |||
| include=include, | |||
| exclude=exclude, | |||
| by_alias=by_alias, | |||
| by_alias=by_alias or False, | |||
There was a problem hiding this comment.
With this trick, we avoid the type when by_alias is None. 😅
| serialize_as_any: bool = False, | ||
| ) -> Dict[str, Any]: | ||
| if PYDANTIC_MINOR_VERSION < (2, 11): | ||
| by_alias = by_alias or False |
There was a problem hiding this comment.
With this we can handle older Pydantic versions but keep the most recent signature.
Recent PRs have been failing due to an issue with the linting step in the CI - I think caused by the recent release of Pydantic 2.11.
Overview of issues and proposed fixes:
get_model_fieldsin_compat.py.I think it's fine to ignore this one?
model_fieldsinSQLModelMetaclasson the other hand, now can do without an ignore warningSQLModel.model_validate()is supposed to overwriteBaseModel.model_validate()but an error gets raised because of theupdateinput parameter, which is not present inBaseModel.model_validate(). The proposed solution here is to work via**kwargs, which I'm generally not a fan of, but the only other option I would see is adding yet another ignore, which feels wrong in this case.SQLModel.model_dump()is supposed to overwriteBaseModel.model_dump()but an error gets raised because ofcontextandaliashaving a wrong type, andfallbacknot being present.fallbackwas introduced in Pydantic v2.11. So instead of restricting Pydantic, I suggest to (again) work via**kwargs.The linting test is failing for Python 3.8 in different ways than for all other Python versions, probably because Python 3.8 support was removed for Pydantic 2.11. Accordingly, I've updated the
test.ymlto skip the linting test for Python 3.8. This doesn't feel like a clean solution either, and I guess the other option is to (also) drop support for Python 3.8.