-
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
docs(api): new ProtocolContext.deck
behavior for multi-slot modules
#14523
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.2.0 #14523 +/- ##
====================================================
Coverage 67.79% 67.79%
====================================================
Files 2518 2518
Lines 72459 72459
Branches 9276 9276
====================================================
Hits 49123 49123
Misses 21118 21118
Partials 2218 2218
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.
Aside from some small amount of confusion about the text related to the deck
property, this looks fine.
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.
Looks great!
Just a question about whether we document old vs new behavior, or just state the latest behavior.
@@ -1085,6 +1092,9 @@ def deck(self) -> Deck: | |||
reflect the new deck state, add a :py:meth:`.pause` or use | |||
:py:meth:`.move_labware` instead. | |||
|
|||
.. versionchanged:: 2.14 | |||
Includes the Thermocycler in all of the slots it occupies. |
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.
Should we mention that in prior versions it returns None
for those slots even when they are occupied by the thermocycler?
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.
Sure, I'll put that in the docstring body (to keep this directive short).
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.
LGTM if you want the extra 2 cents.
🪙
Overview
Follow up to dev work in #14491.
Test Plan
Check the sandbox.
Changelog
ProtocolContext.deck
Review requests
Would like an all clear on removing the known issue. You could still get in a bad jam on 7.1.x.
Risk assessment
v low, docs