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): Refactor instrument-context docstrings #13780

Merged
merged 41 commits into from
Oct 26, 2023

Conversation

jwwojak
Copy link
Contributor

@jwwojak jwwojak commented Oct 12, 2023

Overview

This PR is for managing suggested revisions to the docstrings in instrument_context.py. This is descriptive, customer/user-facing text that appears in Protocols and Instruments section of the API v2 reference. JIRA issue is RTC-204.

These are lightweight changes that revise text, grammar, style, punctuation. Changes also include anchors to longer, more expository API docs throughout because many of the docstrings are terse and/or brief. Linking to other docs helps provide readers with more context, information, code samples, and broader understanding of our API methods. In all cases, we've tried to change as little text as possible.

Excluded: any text related to warnings and errors (raises, runtime, type, etc.) or anything that is a combination of static text and dynamic code. Just revisions to main definitions text only.

Test Plan

Sandbox: http:https://sandbox.docs.opentrons.com/instrument-context-docstrings/v2/new_protocol_api.html#

Testing consisted of making text revisions, pushing to the working branch, and reviewing text results in the sandbox.

Changelog

Definition text changes throughout. Things like:

  • Align text with corresponding method descriptions in the larger API docs.
  • Adding anchor links to other sections.
  • Punctuation (sentences ended without periods)
  • Replacing units of measurement in text with abbreviations (µL replaces "microliter") and getting measurements styled properly (e.g. "20 mm" replaces "20mm").
  • Really tried to be as conservative/light-touch as possible throughout.

Review requests

A lot of this is small and intricate. Any second (or third or n) set of eyes reviewing helps.

Risk assessment

Low, docs/text change only, at least I hope I've only changed text and not working code. heh.

@jwwojak jwwojak added docs papi-v2 Python API V2 labels Oct 12, 2023
@jwwojak jwwojak requested a review from ecormany October 12, 2023 20:18
@jwwojak jwwojak self-assigned this Oct 12, 2023
@jwwojak
Copy link
Contributor Author

jwwojak commented Oct 18, 2023

@ecormany

Picking up and trying again. Ran Black. Output suggests this might be ok:

joewojak@admins-Air-2 protocol_api % black instrument_context.py --diff --color
All done! ✨ 🍰 ✨
1 file would be left unchanged.

Also, resolved small text conflict around air_gap.

Line count doesn't change
@ecormany
Copy link
Contributor

Picking up and trying again. Ran Black. Output suggests this might be ok:

yep, and the "API test/lint/deploy" tests are passing in CI ✅

@jwwojak jwwojak marked this pull request as ready for review October 18, 2023 17:35
@jwwojak jwwojak requested a review from a team as a code owner October 18, 2023 17:35
@jwwojak
Copy link
Contributor Author

jwwojak commented Oct 18, 2023

Changing status from draft.

Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is big, so I left lots of comments. Mostly small stuff on the edited parts. A few methods and parameters were passed over entirely and need a first round of edits.

api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
@@ -1524,7 +1531,7 @@ def hw_pipette(self) -> PipetteDict:
@property # type: ignore
@requires_version(2, 0)
def channels(self) -> int:
"""The number of channels on the pipette."""
"""The number of channels on the pipette. See also, :ref:`new-pipette`."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add that it's 1, 8, or 96?


The Flex 1-Channel 50 µL and Flex 8-Channel 50 µL pipettes must operate in a low-volume mode
to accurately dispense 1 µL of liquid. Low-volume mode can only be set by calling this function.
"""Configure a pipette to handle a specific volume of liquid, measured in µL. Depending on the volume, the pipette will enter a unique pipetting mode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure i like "unique" here. there's two modes, and that makes it sound like there's a lot more. stet or The pipette enters a volume mode depending on the volume provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised using the recommended text.


For more information on available modes for certain pipettes, see :ref:`pipette-volume-modes`.
The Flex 1-Channel 50 µL and Flex 8-Channel 50 µL pipettes must operate in a low-volume mode to accurately dispense 1 µL of liquid. Low-volume mode can
only be set by calling ``configure_for_volume``. See also, :ref:`pipette-volume-modes`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the demonstrative pronoun better. indicates "you are already here". i could see reading this and thinking "oh i have to look up another function"

api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
jwwojak and others added 3 commits October 20, 2023 15:14
Making batch commits before other changes. Will do a batch commit, pull locally, more revisions, and push again.

Co-authored-by: Ed Cormany <[email protected]>
For text like "See, . . . " and "See also, . . . " let's get rid of the commas.

Also, most, if not all, of these should be just "See" not "See also" because we're referring to stuff that directly supports the text.
Single quotes made code italics. Replace with ``code`` to get monospace.

Remove * as a multiplication operator. Replace with "multiplied by".

Fix code sample in dispense.

Run Black check. Looks ok.
Incorporating reviewer comments.
- aspirate
- dispense
- reset_tipracks
- touch_tip
- remove (s)
- small adjustments

Not finished yet. More to come.
@jwwojak jwwojak added api Affects the `api` project and removed papi-v2 Python API V2 labels Oct 24, 2023
- Replace (text only) "tiprack" with "tip rack"
- Fix single quotes around code elements. Replace with double single quotes (`not this`, but ``this instead``). Rendered text in italics.
- drop_tip revisions
- mount revisions, add "left" and "right"
- min_volume revision, add definition
- configure_for_volume changes
Another instance of italics instead of monospace.
@jwwojak jwwojak requested a review from ecormany October 25, 2023 18:32
@jwwojak
Copy link
Contributor Author

jwwojak commented Oct 25, 2023

I've seen formatting issues in the labware class too, but that's out of scope for this PR, which covers the InstrumentContext.

For example columns_by_name, code syntax using single quotes.

Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The substance is good; all that remains is pedantry! I've left many comments to make a record of why certain changes are necessary. I'll batch up the suggestions into a commit and then also do a final pass through to clean up line lengths. Then this will be ready to merge.

api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
ecormany and others added 2 commits October 26, 2023 15:40
and run Black.

All done! ✨ 🍰 ✨
1 file reformatted.
@jwwojak jwwojak merged commit a034c6e into edge Oct 26, 2023
25 checks passed
@jwwojak jwwojak deleted the instrument-context-docstrings branch October 26, 2023 21:04
ecormany added a commit that referenced this pull request Oct 27, 2023
Following up on #13780 with some additional improvements to docstrings within InstrumentContext.
caila-marashaj pushed a commit that referenced this pull request Oct 31, 2023
Following up on #13780 with some additional improvements to docstrings within InstrumentContext.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants