-
Notifications
You must be signed in to change notification settings - Fork 2
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
Split robot load into it's own plan #491
Conversation
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.
can we please rename this so that it's test_robot_load_then_centre
instead of test_wait_for...
?
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.
I'm not massively keen on the RobotLoadAndEnergyChange
name, I keep wanting to find a robot_load_and_energy_change plan but there isn't one. I think if it's an important aspect of the plan, the plan ought to include the name, otherwise the plan and the parameter / composite names ought to line up.
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.
You're right. Ideally they should be separate but it's hard to do that whilst we can't do parallel plans. I will make them consistent in saying they do energy changes.
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.
Apart from naming comment, all looks good
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
==========================================
+ Coverage 77.90% 78.01% +0.11%
==========================================
Files 89 91 +2
Lines 6711 6754 +43
==========================================
+ Hits 5228 5269 +41
- Misses 1483 1485 +2
|
Fixes #490 and fixes #302
To test: