-
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
feat(shared-data): add Pydantic models for liquid class schema #16459
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #16459 +/- ##
===========================================
+ Coverage 63.28% 74.72% +11.44%
===========================================
Files 300 42 -258
Lines 15786 3118 -12668
===========================================
- Hits 9990 2330 -7660
+ Misses 5796 788 -5008
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 is good! I like the strict type checking.
Some minor corrections and suggestions for names and descriptions. And the removal of multi-dispense mix
class Coordinate(BaseModel): | ||
"""Three-dimensional coordinates.""" | ||
|
||
x: _Number | ||
y: _Number | ||
z: _Number |
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 use coordinates in a number of models. Wonder if it'll be useful clubbing together shared models of shared data. Just a thought
z: _Number | ||
|
||
|
||
class DelayArguments(BaseModel): |
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.
The schema calls these 'params' while it's 'arguments' here. I think we should use the same thing for both. Was there a particular reason for calling these 'arguments'?
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.
Mostly to avoid confusion with the top level aspirateParams
, singleDepenseParams
, etc. I'd be open to other ideas as long as they disambiguate them with those.
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.
Valid point. How about calling the top-level ones as aspirateProperties
or aspirateArguments
and have the 'params' from schema be 'params' like DelayParams
, TouchTipParams
? That keeps the naming consistent with schema and still keeps the two entities separate.
I've been calling them aspirateProperties
, dispenseProperties
etc in the API side of things.
"""Shared properties for delay..""" | ||
|
||
enable: bool = Field(..., description="Whether delay is enabled.") | ||
params: Optional[DelayArguments] = Field( |
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.
Related to the earlier comment- This feels a bit awkward that the type of delay's params is DelayArguments
and not DelayParams
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
shared-data/python/opentrons_shared_data/liquid_classes/liquid_class_definition.py
Outdated
Show resolved
Hide resolved
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.
Thanks for making the changes! LGTM! 🙌
Overview
Closes AUTH-834.
This PR follows #16267 and implements the schema as shared data Pydantic models for usage in other parts of the codebase. The models follow the schema in a straightforward matter, the only extra parts that needed to be added were custom validators for the
params
portion of the different liquid handling properties. If the params areNone
butenable
is set to True, it will raise a validation error.Test Plan and Hands on Testing
Testing is covered by the unit test added which is able to successfully load the example fixture as the Pydantic model.
Changelog
liquid_class
folder to Python section of shared-dataReview requests
Mostly suggestions for names of files, models, and definitely descriptions. Most of the descriptions written are pretty simple and more useful suggestions would be appreciated.
Risk assessment
Low, new shared data code that doesn't touch anything else.