-
Notifications
You must be signed in to change notification settings - Fork 207
feat(shared-data): add in-schema versions to liquid class schema and definitions #18401
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
Changes from 2 commits
e1be3d9
06dddbf
d964102
fd1f9f5
780c2d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -393,6 +393,9 @@ class LiquidClassSchemaV1(BaseModel): | |
| schemaVersion: Literal[1] = Field( | ||
| ..., description="Which schema version a liquid class is using" | ||
| ) | ||
| version: int = Field( | ||
| ..., description="Version of the liquid class within the schema" | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah. So the version numbers will start over from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI we're not doing it that way for labware definitions. We're doing it like:
Which I'm pretty sure is what we want. Otherwise it's ambiguous what "labware XYZ version 1" refers to, among other problems.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted, I can update the doc string so it doesn't imply otherwise |
||
| namespace: str = Field(...) | ||
| byPipette: Sequence[ByPipetteSetting] = 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.
Wait, why do we want this?
If the user explicitly wants to load
version=456after loadingversion=123, are you saying we have to returnversion=123no matter what?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.
Ooh I did not look closely enough at this test when I was fixing it. The way I have it now is that if you load say version 1 of a liquid class, and then try loading version 2, we will return version 1 because that's what we have cached. This is obviously not ideal, so I will fix that