-
-
Notifications
You must be signed in to change notification settings - Fork 782
✨ Add support for Literal types
#1439
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
Conversation
I have no idea how to add the proper labels to this PR, it looks like the permissions aren't set up to allow me to.
TypeError for fields annotated with Literal (追記ここまで)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@nsaccente, thanks for working on this!
This works, but could you please add a test?
@nsaccente, thanks for working on this! This works, but could you please add a test?
Sure. I'll try to get that done sometime this week.
...del into bugfix-57/literal-satype
2aaef2d to
3c024e2
Compare
I've been fighting with getting the tests to work. I've been able to get things to pass in some versions of python, but not others. Any guidance on how to run these CI tests locally?
@nsaccente: there were some tricky linting errors that I've now fixed.
@YuriiMotov: do you have time to give this another review?
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.
It seems to me that we can simplify things by just importing Literal from typing_extensions directly. Why do we need that trick with if sys.version_info >= (3, 9):?
This comment was marked as resolved.
This comment was marked as resolved.
@YuriiMotov: you're right - it's more clean overall. I'm happy to defer the Literal import to either another PR or to just wait for 3.8 deprecation, at which point we shouldn't forget to clean this up. Let me clean up this PR, addressing your comments.
Co-authored-by: Motov Yurii <109919500+YuriiMotov@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
TypeError for fields annotated with Literal (削除ここまで)Literal types (追記ここまで)
I'm seeing that this assumes all Literal values are strings, so a model like this:
class Hero(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) name: str = Field(unique=True) weakness: Literal[1, 2, 3]
...would still create the column in the DB as a string, not as an integer. 🤔
Things are getting complex.. We can easily add special cases for Literal[1, 2, 3] or Literal[True, False] to create column with proper type.
The question is, do we expect any kind of validation for these values?
Our database column type is just CHAR or INTEGER, so, database will not enforce the value to be correct against Literal[1, 2, 3] type. If we have the value 42 in database, it can break user's code in a very tricky way one day
Need to decide about data validation first
This PR addresses issue 57. The original issue explains that attempting to use a Literal type annotation in a SQLModel raises a type error from
issubclass. This is becauseLiteralis not a class, but a typing._SpecialForm, so static funcs likeisinstanceandissubclassdon't work with it.The fix was pretty straightforward, I just added a check before an
isinstanceorissubclasswould have been called that checksif type_ is Literal.