-
Notifications
You must be signed in to change notification settings - Fork 183
refactor(step-generation): drop tip appropiately #18658
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## chore_release-pd-8.5.0 #18658 +/- ##
===========================================================
+ Coverage 24.11% 57.10% +32.98%
===========================================================
Files 3284 3256 -28
Lines 285508 281367 -4141
Branches 29082 34855 +5773
===========================================================
+ Hits 68853 160661 +91808
+ Misses 216631 120514 -96117
- Partials 24 192 +168
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
@@ -61,8 +67,20 @@ export function quickTransferStepCommands( | |||
? nonLoadCommandCreator.python ?? [] | |||
: [] | |||
|
|||
let finalDropTipCommand = '' | |||
|
|||
if (Object.values(trashBinEntities).length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to add this final drop tip command back since step-generation is shared between QT and PD. Technically, QT can emit keep_last_tip=False
always since it doesn't support new_tip=never
but since PD needs keep_last_tip=True
and i didn't wanna have branching logic, i decided to just add this drop tip atomic command back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See question below:
args.commandCreatorFnName === 'mix' || !willReuseTip | ||
? curryCommandCreator | ||
: curryWithoutPython | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if willReuseTip==true
, why do you want to emit a JSON dropTipInBlah
but omit the Python drop_tip()
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh this is a good catch! this only needs to emit curryCommandCreator
now, i think. this logic is sort of remnants of when i removed emitting drop tip a few days ago 😓
Overview
Note: Jeremy's PR is stil wrapping up but the arg name and its usage in this PR should be correct. You just can't test it with an app built off of this branch.
Now, we emit
drop_tip
at the end of the step based off of the subsequent transfer step, and if there is no subsequent transfer step, we will emit a final drop tip.Test Plan and Hands on Testing
Review code and upload a test protocol to the app, should pass analysis with a subsequent step new tip of
never
Test the protocol you make on Jeremy's PR branch.
Changelog
generateRobotStateTimeline
Risk assessment
low-ish