[refactor] driver hamamatsurx: simplify return type for *ParamsList#3485
Conversation
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.
There was a problem hiding this comment.
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/SeqParamsListto 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]: |
| def MainParamsList(self) -> List[str]: | ||
| """Returns a list of all parameters related to main window. | ||
| This command can be used to build up a complete parameter list related to main window at runtime. | ||
| :returns: NumberOfParameters,Parameter1,..., ParameterN""" | ||
| return self.sendCommand("MainParamsList") | ||
| :return: list of main window parameter names | ||
| """ | ||
| result = self.sendCommand("MainParamsList") | ||
| if not result: | ||
| logging.warning("MainParamsList returned empty result.") | ||
| return [] |
| def GenParamsList(self) -> List[str]: | ||
| """Returns a list of all parameters related to the general options. | ||
| :returns: NumberOfParameters,Parameter1,..., ParameterN.""" | ||
| return self.sendCommand("GenParamsList") | ||
| :return: list of general option parameter names | ||
| """ | ||
| result = self.sendCommand("GenParamsList") | ||
| if not result: | ||
| logging.warning("GenParamsList returned empty result.") | ||
| return [] |
📝 WalkthroughWalkthroughThis PR adds comprehensive typing annotations to the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/odemis/driver/hamamatsurx.py (1)
1678-1686: ⚡ Quick winShared error handling pattern may return malformed data across all
*ParamsListmethods.All six
*ParamsListmethods (MainParamsList,GenParamsList,AcqParamsList,CamParamsList,DevParamsList,SeqParamsList) share the same error handling pattern: when parsingresult[0]as an integer fails, they return the rawresultlist. 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_paramsat line 149). Returning malformed data could cause subtle bugs if the first element is not a parameter name.Recommendation: Consider returning
[]instead ofresultwhen parsing fails, or add comments explaining that returningresultis 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
📒 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.