Skip to content

[refactor] driver hamamatsurx: simplify return type for *ParamsList#3485

Open
pieleric wants to merge 1 commit into
delmic:masterfrom
pieleric:refactor-driver-hamamatsurx-simplify-return-type-for-paramslist
Open

[refactor] driver hamamatsurx: simplify return type for *ParamsList#3485
pieleric wants to merge 1 commit into
delmic:masterfrom
pieleric:refactor-driver-hamamatsurx-simplify-return-type-for-paramslist

Conversation

@pieleric

Copy link
Copy Markdown
Member

Don't return the length of the list, as first argument, but just warn if it's not the
actual length.

There were only 3 callers using the functions, and all had the wrong
expectation.

Copilot AI review requested due to automatic review settings June 11, 2026 07:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the Hamamatsu RemoteEx driver (hamamatsurx.py) so the various *ParamsList() helpers return only the parameter-name list (dropping the leading “reported count”), while still warning when the reported count doesn’t match the received number of parameters.

Changes:

  • Updated MainParamsList/GenParamsList/AcqParamsList/CamParamsList/DevParamsList/SeqParamsList to strip the leading count and log warnings on unexpected/empty/mismatched responses.
  • Added/adjusted return type hints to consistently represent RemoteEx responses as List[str].
  • Improved docstrings to reflect the simplified return contract for *ParamsList().

super().terminate()

def sendCommand(self, func, *args, **kwargs):
def sendCommand(self, func, *args, **kwargs) -> List[str]:
Comment thread src/odemis/driver/hamamatsurx.py Outdated
Comment thread src/odemis/driver/hamamatsurx.py Outdated
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c4d1fe6d-65e6-42ce-a985-cd7c3349de6e

📥 Commits

Reviewing files that changed from the base of the PR and between a8422d7 and b8bb558.

📒 Files selected for processing (1)
  • src/odemis/driver/hamamatsurx.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/odemis/driver/hamamatsurx.py

📝 Walkthrough

Walkthrough

This PR adds comprehensive typing annotations to the StreakCamera class in hamamatsurx.py and standardizes how six parameter list methods (MainParamsList, GenParamsList, AcqParamsList, CamParamsList, DevParamsList, SeqParamsList) parse RemoteEx command responses. A new internal helper _send_params_list_cmd consolidates response parsing logic: methods now return List[str] containing only parameter names, validate the embedded parameter count against the actual list length, emit warnings on mismatches, and return empty lists for null responses. The base sendCommand method gains a List[str] return type, and related parameter query methods (AcqParamInfoEx, CamParamGet) receive explicit input and return type hints.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring: simplifying return types for *ParamsList methods by removing the list length from the first argument.
Description check ✅ Passed The description is related to the changeset, explaining the simplification of return values and the rationale for the change based on caller expectations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/odemis/driver/hamamatsurx.py (1)

1678-1686: ⚡ Quick win

Shared error handling pattern may return malformed data across all *ParamsList methods.

All six *ParamsList methods (MainParamsList, GenParamsList, AcqParamsList, CamParamsList, DevParamsList, SeqParamsList) share the same error handling pattern: when parsing result[0] as an integer fails, they return the raw result list. This could include an unparseable count as the first element, which violates the documented return type of "list of parameter names."

Callers expect only parameter names (e.g., if "TimingMode" in avail_params at line 149). Returning malformed data could cause subtle bugs if the first element is not a parameter name.

Recommendation: Consider returning [] instead of result when parsing fails, or add comments explaining that returning result is intentional defensive programming for devices that might return parameters without a count prefix. The logged warning helps debugging, but an empty list return would be more robust against truly malformed responses.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/odemis/driver/hamamatsurx.py` around lines 1678 - 1686, The six methods
MainParamsList, GenParamsList, AcqParamsList, CamParamsList, DevParamsList, and
SeqParamsList currently return the raw result when int(result[0]) parsing fails;
change them to return an empty list instead to ensure callers always receive a
list of parameter names. In each method replace the "return result" branch
(after catching ValueError/IndexError) with "return []" and keep the logging
warning as-is so malformed device responses are logged but downstream code is
not fed a malformed list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/odemis/driver/hamamatsurx.py`:
- Around line 1678-1686: The six methods MainParamsList, GenParamsList,
AcqParamsList, CamParamsList, DevParamsList, and SeqParamsList currently return
the raw result when int(result[0]) parsing fails; change them to return an empty
list instead to ensure callers always receive a list of parameter names. In each
method replace the "return result" branch (after catching ValueError/IndexError)
with "return []" and keep the logging warning as-is so malformed device
responses are logged but downstream code is not fed a malformed list.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4fc4a0c3-f67e-49ff-b60a-c0dff6d2cf8f

📥 Commits

Reviewing files that changed from the base of the PR and between f74a181 and a8422d7.

📒 Files selected for processing (1)
  • src/odemis/driver/hamamatsurx.py

Don't return the length of the list, as first argument, but just warn if it's not the
actual length.

There were only 3 callers using the functions, and all had the wrong
expectation.
@pieleric pieleric force-pushed the refactor-driver-hamamatsurx-simplify-return-type-for-paramslist branch from a8422d7 to b8bb558 Compare June 17, 2026 21:18
@pieleric pieleric requested a review from nandishjpatel June 17, 2026 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants