Skip to content
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

chore: upgrade to pydantic v2 #86

Merged
merged 2 commits into from
Jan 10, 2024
Merged

chore: upgrade to pydantic v2 #86

merged 2 commits into from
Jan 10, 2024

Conversation

ss2165
Copy link
Member

@ss2165 ss2165 commented Jan 10, 2024

No description provided.

@ss2165 ss2165 requested a review from mark-koch January 10, 2024 14:59
Comment on lines 121 to 123
class Sum(ABC, BaseModel):
"""Sum type, variants are tagged by their position in the type row"""
# class Sum(ABC, BaseModel):
# """Sum type, variants are tagged by their position in the type row"""

t: Literal["Sum"] = "Sum"
# t: Literal["Sum"] = "Sum"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably delete this

Comment on lines 144 to 145
SumUnion = UnitSum | GeneralSum
Sum = Annotated[SumUnion, Field(discriminator="s")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you define the alias SumUnion instead of inlining it below?

Copy link
Member Author

Choose a reason for hiding this comment

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

to use it in in the isinstance check (annotated unions don't work)

Copy link
Member Author

Choose a reason for hiding this comment

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

got rid of it

@@ -24,7 +23,7 @@ class TypeParam(BaseModel):

class BoundedNatParam(BaseModel):
tp: Literal["BoundedNat"] = "BoundedNat"
bound: int | None
bound: int | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this default needed for deserialisation?

Copy link
Member Author

Choose a reason for hiding this comment

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

the migration code added it automatically (bump_pydantic)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I'd remove it then (so that you have to make a conscious choice when passing None)

@ss2165 ss2165 requested a review from mark-koch January 10, 2024 15:16
@@ -24,7 +23,7 @@ class TypeParam(BaseModel):

class BoundedNatParam(BaseModel):
tp: Literal["BoundedNat"] = "BoundedNat"
bound: int | None
bound: int | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I'd remove it then (so that you have to make a conscious choice when passing None)

@@ -18,7 +18,7 @@ class FunctionVal(BaseModel):
"""A higher-order function value."""

v: Literal["Function"] = "Function"
hugr: Any # TODO
hugr: Any = None # TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@ss2165 ss2165 requested a review from mark-koch January 10, 2024 15:37
@ss2165 ss2165 merged commit a585bfb into main Jan 10, 2024
1 check passed
@ss2165 ss2165 deleted the chore/pydantic2 branch January 10, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants