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): Fix description of run log payload #13776

Merged
merged 6 commits into from
Oct 25, 2023
Merged

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 12, 2023

Overview

While working on #13709, I noticed some outdated documentation for opentrons.simulate.simulate() and opentrons.execute.execute().

The docs say you're supposed to do this:

command = ...  # get from opentrons.simulate.simulate() or opentrons.execute.execute()

run_log_text = command["payload"]["text"].format(command["payload"])

However, it's apparently been simplified to this now:

command = ... # get from opentrons.simulate.simulate() or opentrons.execute.execute()

run_log_text = command["payload"]["text"]

I'm not sure how long this has been the case. PR #9799 suggests it's at least since v5.0.2 but doesn't provide further details ("text field is not a format string anymore"). I tried some Git archeology, but I couldn't get to the bottom of it quickly.

Unfortunately, if you try to do the old .format() thing today, it's actively harmful. If the string happens to contain any { or } characters, it will confuse .format() and raise a KeyError. This happens for Thermocycler commands (see issue #9988), and of course it can happen for ProtocolContext.comment() commands.

Test Plan

Changelog

  • Document that you should just do command["payload"]["text"].
  • Document that you should no longer do command["payload"]["text"].format(command["payload"]).

Review requests

  • Do you see any other places where we encourage the .format() thing? Anything in the Support help center? Any demo files floating around?
  • Does my flowery prose inspire joy in the depths of your heart?

Risk assessment

Low.

@SyntaxColoring SyntaxColoring added api Affects the `api` project docs labels Oct 12, 2023
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #13776 (8e96a4b) into edge (c8d3454) will decrease coverage by 0.11%.
Report is 24 commits behind head on edge.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13776      +/-   ##
==========================================
- Coverage   70.66%   70.56%   -0.11%     
==========================================
  Files        2477     2485       +8     
  Lines       69666    69779     +113     
  Branches     8453     8480      +27     
==========================================
+ Hits        49232    49239       +7     
- Misses      18448    18554     +106     
  Partials     1986     1986              
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/execute.py 54.23% <ø> (ø)
...i/src/opentrons/protocol_api/instrument_context.py 87.73% <ø> (ø)
api/src/opentrons/simulate.py 57.73% <ø> (ø)

... and 29 files with indirect coverage changes

@SyntaxColoring SyntaxColoring marked this pull request as ready for review October 20, 2023 15:21
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 20, 2023 15:21
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 looks good to me. The most important part is the inclusion of the new notes to ward people away from writing code that might break. All the little comments are possible improvements, but completely optional.

I would like to rewrite the prose earlier in each of these docstrings, but I think that's already covered in RTC-211.

api/src/opentrons/simulate.py Outdated Show resolved Hide resolved
api/src/opentrons/simulate.py Show resolved Hide resolved
api/src/opentrons/simulate.py Show resolved Hide resolved
api/src/opentrons/simulate.py Outdated Show resolved Hide resolved
api/src/opentrons/execute.py Outdated Show resolved Hide resolved
api/src/opentrons/execute.py Show resolved Hide resolved
api/src/opentrons/execute.py Outdated Show resolved Hide resolved
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.

Added a comment about the use of "deprecated". Does the text need a stronger warning?

@SyntaxColoring
Copy link
Contributor Author

Failing tests appear to all be flaky Thermocycler integration tests.

@SyntaxColoring SyntaxColoring merged commit c2501c3 into edge Oct 25, 2023
19 of 25 checks passed
@SyntaxColoring SyntaxColoring deleted the fix_payload_docs branch October 25, 2023 17:13
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.

3 participants