Open
Conversation
Reviewer's GuideImplements a step_mode feature enabling immediate execution per liquid handling method in PRCXI9320, adds matrices/materials export logic and structured example scripts, and updates the default host address to 192.168.1.201. Sequence diagram for step_mode immediate execution in PRCXI9320sequenceDiagram
participant User
participant Handler
participant Backend
User->>Handler: Call pick_up_tips()
alt step_mode = True
Handler->>Handler: create_protocol()
Handler->>Backend: pick_up_tips()
Handler->>Handler: run_protocol()
else step_mode = False
Handler->>Backend: pick_up_tips()
end
Class diagram for PRCXI9300Handler step_mode changesclassDiagram
class PRCXI9300Handler {
+bool step_mode
+async pick_up_tips(...)
+async aspirate(...)
+async dispense(...)
+async drop_tips(...)
+async discard_tips(...)
+async mix(...)
+async create_protocol(...)
+async run_protocol(...)
}
PRCXI9300Handler --|> SuperHandler
PRCXI9300Handler o-- "1" PRCXI9300Deck
PRCXI9300Handler o-- "1" Backend
Flow diagram for exporting matrices and materials to JSONflowchart TD
A["Create PRCXI9300Api"] --> B["Fetch matrices"]
A --> C["Fetch materials"]
B --> D["Combine matrices and materials"]
C --> D
D --> E["Write to prcxi_layout_and_materials.json"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The step_mode logic is repeated verbatim in multiple methods; consider refactoring it into a shared decorator or helper function to reduce boilerplate.
- In step_mode branches you drop the return values of backend calls, causing inconsistent return types between modes—ensure both branches return the same result.
- The large demo and JSON‐export code in the main block could bloat the library; move examples and one‐off scripts into a separate file or CLI entrypoint instead of a hardcoded flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The step_mode logic is repeated verbatim in multiple methods; consider refactoring it into a shared decorator or helper function to reduce boilerplate.
- In step_mode branches you drop the return values of backend calls, causing inconsistent return types between modes—ensure both branches return the same result.
- The large demo and JSON‐export code in the __main__ block could bloat the library; move examples and one‐off scripts into a separate file or CLI entrypoint instead of a hardcoded flag.
## Individual Comments
### Comment 1
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:342-347` </location>
<code_context>
- return await self._unilabos_backend.mix(
- targets, mix_time, mix_vol, height_to_bottom, offsets, mix_rate, none_keys
- )
+ if self.step_mode:
+ await self.create_protocol(f"单点动作{time.time()}")
+ await self._unilabos_backend.mix(
+ targets, mix_time, mix_vol, height_to_bottom, offsets, mix_rate, none_keys
+ )
+ await self.run_protocol()
+ else:
+ return await self._unilabos_backend.mix(
</code_context>
<issue_to_address>
**issue:** Mix method now has different return behavior in step_mode.
Returning a value in both branches will help prevent unexpected behavior for callers relying on a consistent method interface.
</issue_to_address>
### Comment 2
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:364-368` </location>
<code_context>
async def pick_up_tips(
self,
tip_spots: List[TipSpot],
use_channels: Optional[List[int]] = None,
offsets: Optional[List[Coordinate]] = None,
**backend_kwargs,
):
if self.step_mode:
await self.create_protocol(f"单点动作{time.time()}")
await super().pick_up_tips(tip_spots, use_channels, offsets, **backend_kwargs)
await self.run_protocol()
else:
return await super().pick_up_tips(tip_spots, use_channels, offsets, **backend_kwargs)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>
### Comment 3
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:382` </location>
<code_context>
async def aspirate(
self,
resources: Sequence[Container],
vols: List[float],
use_channels: Optional[List[int]] = None,
flow_rates: Optional[List[Optional[float]]] = None,
offsets: Optional[List[Coordinate]] = None,
liquid_height: Optional[List[Optional[float]]] = None,
blow_out_air_volume: Optional[List[Optional[float]]] = None,
spread: Literal["wide", "tight", "custom"] = "wide",
**backend_kwargs,
):
if self.step_mode:
await self.create_protocol(f"单点动作{time.time()}")
await super().aspirate(
resources,
vols,
use_channels,
flow_rates,
offsets,
liquid_height,
blow_out_air_volume,
spread,
**backend_kwargs,
)
await self.run_protocol()
else:
return await super().aspirate(
resources,
vols,
use_channels,
flow_rates,
offsets,
liquid_height,
blow_out_air_volume,
spread,
**backend_kwargs,
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>
### Comment 4
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:417-422` </location>
<code_context>
async def drop_tips(
self,
tip_spots: Sequence[Union[TipSpot, Trash]],
use_channels: Optional[List[int]] = None,
offsets: Optional[List[Coordinate]] = None,
allow_nonzero_volume: bool = False,
**backend_kwargs,
):
if self.step_mode:
await self.create_protocol(f"单点动作{time.time()}")
await super().drop_tips(tip_spots, use_channels, offsets, allow_nonzero_volume, **backend_kwargs)
await self.run_protocol()
else:
return await super().drop_tips(tip_spots, use_channels, offsets, allow_nonzero_volume, **backend_kwargs)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>
### Comment 5
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:436` </location>
<code_context>
async def dispense(
self,
resources: Sequence[Container],
vols: List[float],
use_channels: Optional[List[int]] = None,
flow_rates: Optional[List[Optional[float]]] = None,
offsets: Optional[List[Coordinate]] = None,
liquid_height: Optional[List[Optional[float]]] = None,
blow_out_air_volume: Optional[List[Optional[float]]] = None,
spread: Literal["wide", "tight", "custom"] = "wide",
**backend_kwargs,
):
if self.step_mode:
await self.create_protocol(f"单点动作{time.time()}")
await super().dispense(
resources,
vols,
use_channels,
flow_rates,
offsets,
liquid_height,
blow_out_air_volume,
spread,
**backend_kwargs,
)
await self.run_protocol()
else:
return await super().dispense(
resources,
vols,
use_channels,
flow_rates,
offsets,
liquid_height,
blow_out_air_volume,
spread,
**backend_kwargs,
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>
### Comment 6
<location> `unilabos/devices/liquid_handling/prcxi/prcxi.py:470-475` </location>
<code_context>
async def discard_tips(
self,
use_channels: Optional[List[int]] = None,
allow_nonzero_volume: bool = True,
offsets: Optional[List[Coordinate]] = None,
**backend_kwargs,
):
if self.step_mode:
await self.create_protocol(f"单点动作{time.time()}")
await super().discard_tips(use_channels, allow_nonzero_volume, offsets, **backend_kwargs)
await self.run_protocol()
else:
return await super().discard_tips(use_channels, allow_nonzero_volume, offsets, **backend_kwargs)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+342
to
+347
| if self.step_mode: | ||
| await self.create_protocol(f"单点动作{time.time()}") | ||
| await self._unilabos_backend.mix( | ||
| targets, mix_time, mix_vol, height_to_bottom, offsets, mix_rate, none_keys | ||
| ) | ||
| await self.run_protocol() |
There was a problem hiding this comment.
issue: Mix method now has different return behavior in step_mode.
Returning a value in both branches will help prevent unexpected behavior for callers relying on a consistent method interface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add step mode feature to 9320:
Add feature to obtain matrices and materials list
Updated host for 9320
Summary by Sourcery
Introduce step_mode for per-action protocol execution, add utility to export matrices and materials as JSON, include usage examples, and update the device host address.
New Features:
Enhancements:
Chores: