-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add type-checking support for SQLModel types. #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work if sqlmodel is not installed. Since it's an optional dependency, it should check if sqlmodel is installed and also work if it's not installed
I'll just leave this here for whoever wants to implement it then. Just needed this for my project. |
Could we just add sqlmodel as regular dependency then? I think that's de facto the suggested library for fastcrud. |
Yeah I did a bit of research into this and the only way I see that we could truly allow it to be optional is to use the |
So I did make the ...
if not isinstance(model, DeclarativeBase):
try:
from sqlmodel.main import SQLModel, SQLModelMetaclass
if not isinstance(model, Union[SQLModel, SQLModelMetaclass]):
raise ValueError(
f"Invalid model type. Expected SQLAlchemy DeclarativeBase or SQLModel, "
f"but got {type(model).__name__}. "
f"Please ensure you're using a valid SQLAlchemy or SQLModel model."
)
except ImportError:
raise ImportError(
f"Invalid model type. Expected SQLAlchemy DeclarativeBase, but got {type(model).__name__}. "
f"To use SQLModel, ensure you have installed the 'sqlmodel' package. "
f"You can install it using 'pip install sqlmodel' or 'poetry add sqlmodel'."
) However, it kept failing tests with random model types which I could manually check for, but am not familiar enough with these libraries to confidently say that would not lead to massive unintended side-effects. Note: |
Also, I would like to suggest adding Makefile to simplify testing, linting, and formatting. I find a universal |
I am highly against this. Make introduces yet another layer of abstraction. We should stick to poetry and docker compose exec's whenever possible. What you might be interrested in is:
|
Something like this would be good indeed. I think we could revisit in the future, but for now it looks good to me. Is it actually working with the example in the issue? |
Ruff does this, |
I agree. The commands are in contributing.md, we could add them to readme as well to make it easier. |
Yes, the issues don't appear for me with my changes with his example code. |
Added Type Checking Support for SQLModel Classes
Description
My changes solve #58 and allow users to directly use SQLModel without type-issues. It passes all tests and shows no typing issues.
Changes
I made many none-obtrusive type changes such as replacing instances of
type[DeclarativeBase]
withtype[Union[DeclarativeBase, SQLModel]]
. This alone caused mypy errors do to table not being present on SQLModel which doesn't make sense as it does, so I think this is just poor typing on SQLModel's side. To deal with this wherever model.title is being referenced I explicitly checked if that field was available and raised a ValueError if it was not. Doing this allowed my changes to pass all linting checks.Checklist