Skip to content

Conversation

bolu61
Copy link

@bolu61 bolu61 commented Oct 29, 2024

Fixes #229 (see #229 (comment)).

I'm not really sure how Pydantic does it, but here's a potential workaround to the problem.

@svlandeg svlandeg added the feature New feature or request label Feb 24, 2025
@svlandeg svlandeg changed the title Add PEP 593 support for Requirement annotations ✨ Add PEP 593 support for Requirement annotations Feb 24, 2025
Copy link
Contributor

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bolu61, thanks for working on this!

So, this PR fixes the following code example:

class Foo(SQLModel, table=True):
    id: Annotated[int | None, Field(primary_key=True)] = None
    uuid: Annotated[UUID, Field(unique=True)]

class Bar(SQLModel, table=True):
    id: Annotated[int | None, Field(primary_key=True)] = None
    uuid: Annotated[UUID, Field(unique=True)]
    foo_id: Annotated[int, Field(foreign_key="foo.id")]
    foo: Annotated[Foo, Relationship()]

For now last line (with Relationship()) doesn't work (ValueError: <class '__main__.Foo'> has no matching SQLAlchemy type) and this PR fixes that.

I think this is not the same that was initially proposed in #229. The idea there was to support passing multiple annotations like:

class Hero(SQLModel, table=True):
    id: Annotated[Optional[int], Field(examples=....), Column(primary_key=True)] = None

Where Field can be pydantic.Field.
So, I suggest we unmark this PR as the one that resolves that issue.

As for changes introduced by this PR, I think it goes in line with modern recommendations to use Annotated approach instead parameterizing fields via default value.
I think we should test this approach more and add some automatic tests. Then it will have all chances to be accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

open ended discussion: use of PEP 593 in the "model" ecosystem
3 participants