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): Disallow direct access to .state through Protocol Engine state views #17016

Merged
merged 7 commits into from
Dec 4, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 3, 2024

Overview

A trivial refactor to avoid directly accessing private implementation details.

Details

In opentrons.protocol_engine.state, in a FooStore+FooView+FooState triad, we generally want to treat FooState as a private implementation detail of FooStore+FooView. For example, we don't want command implementations to access it, and we especially don't want Python-Protocol-API–level code to access it. One reason is encapsulation, and another reason is to prevent that code from mutating FooState, which would be unsafe.

However, it looks like we have been exposing FooState through an inherited FooView.state property. A handful of command implementations and Python Protocol API bits were using it. I think this was accidental? It was certainly surprising to me.

This replaces those FooView.state accesses with public FooView methods and removes access to FooView.state.

Test Plan and Hands on Testing

None needed.

Review requests

Is this what we want?

Risk assessment

Low.

@SyntaxColoring SyntaxColoring requested review from a team December 3, 2024 18:36
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner December 3, 2024 18:36
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.

Looks good, but why remove that HasState?

@SyntaxColoring
Copy link
Contributor Author

why remove that HasState?

That's what was exposing the state. HasState defines a state @property, and each view was inheriting it from there.

@SyntaxColoring SyntaxColoring merged commit 02a7bfa into edge Dec 4, 2024
27 checks passed
@SyntaxColoring SyntaxColoring deleted the remove_exposed_state_property branch December 4, 2024 15:17
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