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

refactor(hardware): move scheduler #13026

Closed
wants to merge 11 commits into from

Conversation

ahiuchingau
Copy link
Contributor

Overview

Test Plan

Changelog

Review requests

Risk assessment

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #13026 (e227728) into edge (b59c20d) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13026      +/-   ##
==========================================
- Coverage   72.45%   72.42%   -0.03%     
==========================================
  Files        2388     2390       +2     
  Lines       66012    66029      +17     
  Branches     7319     7332      +13     
==========================================
- Hits        47829    47824       -5     
- Misses      16443    16466      +23     
+ Partials     1740     1739       -1     
Flag Coverage Δ
app 71.25% <ø> (+<0.01%) ⬆️
components 63.50% <ø> (-1.00%) ⬇️
notify-server 89.13% <ø> (ø)
react-api-client 64.25% <ø> (ø)
shared-data 78.29% <ø> (ø)
system-server 96.07% <ø> (ø)
update-server 64.64% <ø> (ø)
usb-bridge 81.35% <ø> (ø)

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

Impacted Files Coverage Δ
.../opentrons_hardware/firmware_bindings/constants.py 100.00% <ø> (ø)
...e/opentrons_hardware/hardware_control/constants.py 100.00% <ø> (ø)
...rons_hardware/hardware_control/gripper_settings.py 0.00% <ø> (ø)
hardware/opentrons_hardware/scripts/gripper.py 0.00% <ø> (ø)
hardware/opentrons_hardware/scripts/gripper_jaw.py 0.00% <ø> (ø)

... and 5 files with indirect coverage changes

@ahiuchingau ahiuchingau marked this pull request as ready for review July 3, 2023 16:01
@ahiuchingau ahiuchingau requested a review from a team as a code owner July 3, 2023 16:01
@ahiuchingau ahiuchingau marked this pull request as draft July 3, 2023 16:41
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, but I think for readability wins it has to be taken a bit farther - get the runner all the way out of the business of iterating through things based on node+seq; maybe make that the responsibility of something new replacing self._moves or something. Ideally we should start with the concept of factoring out some chunk of responsibility and making it a separately-testable thing.

Also, we should wait until 0.13.0 is in to merge thi; there's a lot of changes to this code in there.

@ahiuchingau ahiuchingau force-pushed the refactor_hardware-move-scheduler branch 2 times, most recently from 9c1003e to 8a58315 Compare July 13, 2023 21:42
@ahiuchingau ahiuchingau force-pushed the refactor_hardware-move-scheduler branch from 8a58315 to e227728 Compare July 13, 2023 22:21
@ahiuchingau ahiuchingau changed the base branch from internal-release_0.13.0 to edge July 13, 2023 22:22
@ahiuchingau
Copy link
Contributor Author

Bit off too much by trying to put everything in this giant refactor - closing this and splitting it up into smaller PRs instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants