-
Notifications
You must be signed in to change notification settings - Fork 179
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(api): Delete dead PipetteStore code and type nozzle maps as non-Optional #16481
Conversation
This reverts commit cb1f6396bdd851d4e61fbaadf93fef12a78e9cb0.
static_config = self._state.static_config_by_id.get(pipette_id) | ||
if static_config: | ||
self._state.nozzle_configuration_by_id[ | ||
pipette_id | ||
] = static_config.default_nozzle_map |
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.
Because of the if static_config:
line, this chunk of code in _set_load_pipette()
depended on running after _update_pipette_config()
. We do not obey that order, so it looked at first like this was broken.
However, it turns out _update_pipette_config()
also sets this attribute, and _update_pipette_config()
is always called in practice whenever _set_load_pipette()
is called. So it's moot.
For clarity, therefore, delete this code. Each attribute of self._state
is now affected either by _set_load_pipette()
or _update_pipette_config()
, never both.
Relying on _update_pipette_config()
always being called in practice whenever _set_load_pipette()
is called feels a little icky to me. I think we can upgrade that from being "in practice" to being "as guaranteed by the types" by rearranging StateUpdate
a little bit, but I'm not doing that here.
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.
Definitely the way to go, thank you!
Overview
Minor refactors to
PipetteStore
to follow up on #16469 (comment) and close EXEC-768.Test Plan and Hands on Testing
KeyError
or anything.Changelog
PipetteState
was typed to allow a nozzle map ofNone
. Since #14529, this never occurs in practice—a properly-loaded pipette always has a nozzle map. So, remove theNone
possibility and simplify the consumers accordingly.PipetteStore
tests to more closely reflect howLoadPipetteImplementation
uses it.Review requests
Does my comment below seem right?
Risk assessment
Low if I do the manual testing described above.