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

docs(api): new ProtocolContext.deck behavior for multi-slot modules #14523

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

ecormany
Copy link
Contributor

Overview

Follow up to dev work in #14491.

Test Plan

Check the sandbox.

Changelog

  • cherrypick commit to keep 2.17 docs out of 7.2.0 feature branch
  • remove known issue about TC leaving slot A1 open on Flex
  • expand docstring for 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

@ecormany ecormany added docs papi-v2 Python API V2 labels Feb 20, 2024
@ecormany ecormany requested a review from a team as a code owner February 20, 2024 15:37
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ad6c1a2) 67.79% compared to head (ca65200) 67.79%.
Report is 9 commits behind head on chore_release-7.2.0.

Additional details and impacted files

Impacted file tree graph

@@                 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           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...i/src/opentrons/protocol_api/instrument_context.py 89.09% <ø> (ø)
api/src/opentrons/protocol_api/protocol_context.py 92.80% <ø> (ø)

Copy link
Contributor

@jwwojak jwwojak left a 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.

Copy link
Member

@sanni-t sanni-t left a 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.
Copy link
Member

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?

Copy link
Contributor Author

@ecormany ecormany Feb 21, 2024

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).

Copy link
Contributor

@jwwojak jwwojak left a 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.

🪙

@ecormany ecormany merged commit f30f52a into chore_release-7.2.0 Feb 21, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs papi-v2 Python API V2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants