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(api): Delete dead PipetteStore code and type nozzle maps as non-Optional #16481

Merged
merged 10 commits into from
Oct 15, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 14, 2024

Overview

Minor refactors to PipetteStore to follow up on #16469 (comment) and close EXEC-768.

Test Plan and Hands on Testing

  • Run something with and without partial tip configuration and make sure it doesn't raise a KeyError or anything.

Changelog

  • PipetteState was typed to allow a nozzle map of None. Since #14529, this never occurs in practice—a properly-loaded pipette always has a nozzle map. So, remove the None possibility and simplify the consumers accordingly.
  • As described in EXEC-768, there was a chunk of code that looked like we recently broke it. It turns out we did not break it, we made it redundant. So just delete it. See my comment below for details.
  • Update PipetteStore tests to more closely reflect how LoadPipetteImplementation uses it.

Review requests

Does my comment below seem right?

Risk assessment

Low if I do the manual testing described above.

@SyntaxColoring SyntaxColoring changed the title refactor(api): Non optional nozzle map refactor(api): Delete dead PipetteStore code and type nozzle maps as non-Optional Oct 14, 2024
Comment on lines -170 to -174
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
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Oct 14, 2024

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.

@SyntaxColoring SyntaxColoring marked this pull request as ready for review October 14, 2024 19:33
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 14, 2024 19:33
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.

Definitely the way to go, thank you!

@SyntaxColoring SyntaxColoring merged commit 1edb0fb into edge Oct 15, 2024
21 checks passed
@SyntaxColoring SyntaxColoring deleted the non_optional_nozzle_map branch October 15, 2024 14:40
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