-
Notifications
You must be signed in to change notification settings - Fork 178
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(pydantic): V1->V2 migration #14805
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #14805 +/- ##
==========================================
- Coverage 67.24% 67.20% -0.05%
==========================================
Files 2495 2495
Lines 71254 71396 +142
Branches 8937 8984 +47
==========================================
+ Hits 47918 47979 +61
- Misses 21235 21302 +67
- Partials 2101 2115 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
offset: Optional[OffsetVector] | ||
profile: Optional[List[ProfileStep]] | ||
radius: Optional[float] | ||
slotName: Optional[str] = None |
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.
we sure about these changes? slotName: Optional[str]
without the =None
means "you have to explicitly specify it but you can specify it as None
/null
" which I think is what we want
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.
In Pydantic v1, slotName: Optional[str]
defaulted to None
when the field was missing from the source JSON:
>>> class M(pydantic.BaseModel):
... slotName: typing.Optional[str]
...
>>> M.parse_raw("{}")
M(slotName=None)
In Pydantic v2, that becomes a validation error. If you want a default, you have to specify a default. This is a good, but breaking, change, so I guess the upgrade script applies this fixup liberally.
In this particular case, with the Params
class, I think it's correct for these to default to None
. In other cases of f: Optional[T]
, though, we'll definitely need to be careful to cross-check against our underlying JSON schemas.
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 mostly makes sense to me so far, thanks!
class Config: | ||
json_encoders = { | ||
# TODO[pydantic]: The following keys were removed: `json_encoders`. | ||
# Check https://docs.pydantic.dev/dev-v2/migration/#changes-to-config for more information. | ||
model_config = ConfigDict( | ||
json_encoders={ | ||
pip_types.PipetteChannelType: lambda v: v.value, | ||
pip_types.PipetteModelType: lambda v: v.value, | ||
pip_types.PipetteGenerationType: lambda v: v.value, | ||
pip_types.Quirks: lambda v: v.value, | ||
} | ||
) |
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 looks like this is saying json_encoders
isn't doing anything anymore; this is dead code.
According to the migration guide, there's some other way to do this now. But it isn't clear to me what this was accomplishing before. These lambdas look like straight passthroughs to me.
diameter: Optional[float] | ||
yDimension: Optional[float] | ||
xDimension: Optional[float] | ||
diameter: Optional[float] = None | ||
yDimension: Optional[float] = None | ||
xDimension: Optional[float] = None |
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 whole WellDefinition
model is apparently unused. I think we can delete it.
da2f913
to
e83d1b5
Compare
e83d1b5
to
332355e
Compare
Superseded by #14871.