feat(api): add per destination option to LC based transfer#18261
feat(api): add per destination option to LC based transfer#18261
Conversation
jbleon95
left a comment
jbleon95
left a comment
There was a problem hiding this comment.
This looks good to me, I'd maybe add some unit tests to make sure consolidate and distribute raise if given per destination.
As for whether we should allow this with distribute and per source for consolidate, I'd say let's hold off on considering adding that for now, since it's not necessary for PD compatibility and we're not losing any functionality.
Can @ncdiehl11 weigh in? I don't remember whether he said the PD consolidate/distribute is exactly like |
| add_final_air_gap=True, | ||
| trash_location=mock.ANY, | ||
| ), | ||
| mock.call.aspirate_liquid_class( |
There was a problem hiding this comment.
Hey, what is this test supposed to do? If there are 2 destination wells, should it have picked up tips twice if you said "per destination"?
There was a problem hiding this comment.
Yep, and it does. This list is 'expected calls per tip'. The assert in the bottom checks that there's two calls. I didn't want to add 50 more lines duplicating the calls if I could avoid it.
PD does not allow users to configure a |
| ) | ||
| ): | ||
| if prev_src is not None: | ||
| if prev_src is not None and prev_dest is not None: |
There was a problem hiding this comment.
I think this is fine. But since you have an and here, is there ever a case where prev_source is None but prev_dest is not None?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18261 +/- ##
=======================================
Coverage 23.62% 23.62%
=======================================
Files 3055 3055
Lines 255734 255734
Branches 30114 30114
=======================================
Hits 60419 60419
Misses 195301 195301
Partials 14 14 🚀 New features to boost your workflow:
|
Closes AUTH-1784 # Overview Adds a 'per destination' tip policy to `transfer_with_liquid_class` only. Doesn't add it to LC based distribute or consolidate. ## Risk assessment Low. Adds a new tip handling logic that shouldn't affect any existing behavior.
Closes AUTH-1784 # Overview Adds a 'per destination' tip policy to `transfer_with_liquid_class` only. Doesn't add it to LC based distribute or consolidate. ## Risk assessment Low. Adds a new tip handling logic that shouldn't affect any existing behavior.
Closes AUTH-1784 # Overview Adds a 'per destination' tip policy to `transfer_with_liquid_class` only. Doesn't add it to LC based distribute or consolidate. ## Risk assessment Low. Adds a new tip handling logic that shouldn't affect any existing behavior.
Closes AUTH-1784 # Overview Adds a 'per destination' tip policy to `transfer_with_liquid_class` only. Doesn't add it to LC based distribute or consolidate. ## Risk assessment Low. Adds a new tip handling logic that shouldn't affect any existing behavior.
Closes AUTH-1784 # Overview Adds a 'per destination' tip policy to `transfer_with_liquid_class` only. Doesn't add it to LC based distribute or consolidate. ## Risk assessment Low. Adds a new tip handling logic that shouldn't affect any existing behavior.
4 participants
Closes AUTH-1784
Overview
Adds a 'per destination' tip policy to
transfer_with_liquid_classonly. Doesn't add it to LC based distribute or consolidate.Test Plan and Hands on Testing
Added integration test.
Review requests
per destinationfordistribute_with_liquid_classandper sourceforconsolidate_with_liquid_class? Because the tip renewing behavior is pretty well-defined and useful for these above combinations (as opposed to havingalways/per sourcefor distribution oralways/per_destinationfor consolidation, which don't make total sense). That said, these cases are actually covered bytransfer_with_liquid_classwith aper sourceorper destinationpolicy so addingper source,per destinationtoconsolidate_with_liquid_classanddistribute_with_liquid_classrespectively doesn't add any new value.Risk assessment
Low. Adds a new tip handling logic that shouldn't affect any existing behavior.