opentrons icon indicating copy to clipboard operation
opentrons copied to clipboard

feat(protocol-engine): Add in-place variants of pipetting commands

Open SyntaxColoring opened this issue 3 years ago • 2 comments

Closes RSS-64.

To do:

  • Update our Decoy version to take advantage of an async thing
  • Other commands (aspirate, etc.) in this PR?
  • Add tests for new commands in JSONCommandTranslator
  • Update JSON protocol schema v6
    • Actually, should we still be updating v6, at this point? Does this warrant a v7?

Overview

Changelog

Review requests

Risk assessment

SyntaxColoring avatar Aug 11 '22 21:08 SyntaxColoring

Codecov Report

Merging #11332 (0fc216e) into edge (fdb72e3) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #11332   +/-   ##
=======================================
  Coverage   74.61%   74.61%           
=======================================
  Files        2045     2045           
  Lines       56957    56957           
  Branches     5543     5543           
=======================================
  Hits        42501    42501           
  Misses      13214    13214           
  Partials     1242     1242           
Flag Coverage Δ
notify-server 89.17% <ø> (ø)

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

Impacted Files Coverage Δ
...src/opentrons/protocol_engine/commands/__init__.py 100.00% <100.00%> (ø)
...entrons/protocol_engine/commands/command_unions.py 100.00% <100.00%> (ø)
...src/opentrons/protocol_engine/commands/dispense.py 100.00% <100.00%> (ø)
...c/opentrons/protocol_engine/execution/pipetting.py 100.00% <100.00%> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <100.00%> (ø)

codecov[bot] avatar Aug 11 '22 21:08 codecov[bot]

LGTM so far, the only comment I have for now is to not leave out updating the PipetteStore with the volume from DispenseInPlaceResult (and future InPlaceResults)

jbleon95 avatar Aug 12 '22 15:08 jbleon95

Do we need to add an integration test?

I was conflicted about this, but I'm going to say "not yet." Because the server technically isn't supposed to accept this command until it gets added to the JSON protocol schema. We're adding the implementation now just so we have less work to do later.

When we officially add this to the JSON schema later, then yeah, we'll want integration tests to match the other commands.

SyntaxColoring avatar Aug 18 '22 15:08 SyntaxColoring