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 obsolete todo comments #17554

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Feb 19, 2025

Overview

Delete some todo comments in the loadLabware and loadLid Protocol Engine command implementations.

Emphasis mine:

these validation checks happen after the labware is loaded, because they rely on the definition. In practice this will not cause any issues since they will raise protocol ending exception, but for correctness should be refactored to do this check beforehand.

As far as I can tell, this is not true. These checks happen inside Protocol Engine command implementations, which, architecturally, commit all their changes only at the very end, once they've completed without errors.

These comments may have been inherited from opentrons.protocol_api-level code, which does still have this problem. They may also have been inherited from older hacky Protocol Engine code that did directly modify state in the middle of a command, code that I believe and hope we've stamped out by now.

Test Plan and Hands on Testing

None needed.

Review requests

Am I correct, or is there something weird about these commands where they do modify Protocol Engine state before they complete?

Risk assessment

No risk.

@SyntaxColoring SyntaxColoring requested a review from a team as a code owner February 19, 2025 17:23
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.

No, I think you're right. I think equipment.load_labware used to also emplace the labware in the state store and no longer does.

@SyntaxColoring SyntaxColoring changed the title refactor(api): Delete todo comments todo comments. refactor(api): Delete obsolete todo comments Feb 19, 2025
@SyntaxColoring SyntaxColoring merged commit f34756f into edge Feb 19, 2025
30 checks passed
@SyntaxColoring SyntaxColoring deleted the delete_obsolete_todos branch February 19, 2025 23:21
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