Skip to content

refactor(api): Delete dead PipetteStore code and type nozzle maps as non-Optional#16481

Merged
SyntaxColoring merged 10 commits intoedgefrom
non_optional_nozzle_map
Oct 15, 2024
Merged

refactor(api): Delete dead PipetteStore code and type nozzle maps as non-Optional#16481
SyntaxColoring merged 10 commits intoedgefrom
non_optional_nozzle_map

Conversation

@SyntaxColoring
Copy link
Copy Markdown
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
Copy Markdown
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
@SyntaxColoring SyntaxColoring requested review from a team, CaseyBatten and sanni-t October 14, 2024 19:34
Copy link
Copy Markdown
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
@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