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

style(api): Revise execute/simulate reference docstrings #15901

Merged
merged 7 commits into from
Aug 6, 2024

Conversation

SyntaxColoring
Copy link
Contributor

Overview

Minor adjustments to the reference docstrings in opentrons.simulate and opentrons.execute.

Test Plan and Hands on Testing

I've skimmed the generated docs and made sure this doesn't introduce any flagrant formatting problems.

Changelog

  • Note, for a couple of arguments, that they are only intended for internal Opentrons use.
  • Adjust indentation so we don't have to describe every parameter with ~50-character-long lines. (No user-visible difference, just changing the formatting in the .py file.)
  • A couple other tiny fixups, like Python instead of python.

Review requests

Is chore_release-8.0.0 the correct merge target?

Risk assessment

No risk.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner August 6, 2024 16:33
@SyntaxColoring SyntaxColoring changed the title docs(api): Revise Simulate docstrings docs(api): Revise execute/simulate reference docstrings Aug 6, 2024
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Yeah, that looks like a good set of changes

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.

Looks good, only improves things — although I spotted a couple more formatting changes you could add.

Maybe the rare opportunity for "style(api)" in the PR title.

api/src/opentrons/execute.py Outdated Show resolved Hide resolved
api/src/opentrons/execute.py Outdated Show resolved Hide resolved
api/src/opentrons/simulate.py Outdated Show resolved Hide resolved
@SyntaxColoring SyntaxColoring changed the title docs(api): Revise execute/simulate reference docstrings style(api): Revise execute/simulate reference docstrings Aug 6, 2024
@SyntaxColoring
Copy link
Contributor Author

Maybe the rare opportunity for "style(api)" in the PR title.

oooo rare commit tag, gimme

@SyntaxColoring SyntaxColoring merged commit e2d48dc into chore_release-8.0.0 Aug 6, 2024
21 checks passed
@SyntaxColoring SyntaxColoring deleted the simulate_docstrings branch August 6, 2024 20:55
Comment on lines +153 to +154
protocol. This is preparation for a beta feature
and is best not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Guys, I'm not sure where or how this text appears, but would recommend changing the last part of the sentence "and is best not used" to something like "and should not be used."

Or

Try 2 shorter sentences, "This is in preparation for a beta feature. Do not use it."

protocol. Note that if you specify this, *only*
labware in this argument will be allowed in the
protocol. This is preparation for a beta feature
and is best not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another "best not used" phrase. From my suggestion above, I'd recommend changing that part of the sentence to something like "and should not be used."

Or, try 2 shorter sentences, "This is in preparation for a beta feature. Do not use it." A stronger admonition if people shouldn't use this.

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.

:shipit: Added some suggestions for consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants