refactor(robot-server): Type-check robot_server.robot #17534
Conversation
| # todo(mm, 2025-02-14): These return types are the source of a lot of ignored and | ||
| # asserted-away type errors in this file. Double-check that they're actually correct. | ||
| def _get_current_calibrations( | ||
| self, | ||
| ) -> tuple[ | ||
| cal_models.DeckCalibrationModel | None, | ||
| dict[Mount, cal_models.InstrumentOffsetModel | None], | ||
| dict[Mount, cal_models.TipLengthModel | None], | ||
| ]: |
There was a problem hiding this comment.
It smells to me like this is "supposed to be" returning PipetteOffsetCalibration instead of InstrumentOffsetModel, and TipLengthCalibration instead of TipLengthModel. That would explain the # type: ignore[attr-defined]s that I've had to add in this file.
But these types follow directly from the calls to get_pipette_offset() and self._get_tip_length_from_pipette(), so 🤷
There was a problem hiding this comment.
Similarly, if the None possibilities were removed, it would resolve a lot.
Resolve conflicts in: * robot-server/robot_server/robot/calibration/check/user_flow.py * robot-server/robot_server/robot/calibration/deck/user_flow.py * robot-server/robot_server/robot/calibration/pipette_offset/user_flow.py * robot-server/robot_server/robot/calibration/tip_length/user_flow.py * robot-server/robot_server/robot/calibration/util.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17534 +/- ##
=======================================
Coverage 26.16% 26.16%
=======================================
Files 2838 2838
Lines 217729 217730 +1
Branches 9280 9281 +1
=======================================
+ Hits 56970 56972 +2
+ Misses 160742 160741 -1
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. |
Overview
robot_server.robot.*is old code that was excluded from type-checking. This removes the exclusion and fixes the type-checking errors, to the extent that I could without disturbing the code too much.This helps with EXEC-1206 and EXEC-1230.
Test Plan and Hands on Testing
I think we're good with CI and careful code review, but if that makes anyone nervous, I can smoke-test the OT-2's calibration flows next week.
Review requests
robot-server/robot_server/robot/calibration/check/user_flow.pywas pretty bad. There are a lot of things there that look like they're forgetting to handle the possibility of aNonevalue, and a lot of things that look like they're accessing properties that don't exist (!). I don't know how, or even if, it was working at all.I’ve left comments in the code for those problems, and a GitHub review comment below. Anyone have insight into them?
Risk assessment
Low—I tried to avoid disrupting the code too much.
I added a number of
assert foo is not Nones, but only where we were accessing properties likefoo.bar, so at worst, it will turn anAttributeErrorinto anAssertionError.