- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
PythonPanelService.GetValue() and .TryGetValue() #112
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
Conversation
| Test Results   10 files  ± 0     10 suites  ±0   22s ⏱️ -9s Results for commit 628d03a. ± Comparison against base commit 098c4b2. This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
A concise description of the purpose of the PR, followed by summarized bullets of changes
- Introduce a non-throwing TryGetValue RPC alongside the existing GetValue RPC
- Update client code and test fixtures to call try_get_valueand handle missing values without exceptions
- Revise gRPC stubs, servicer, and proto definitions to include TryGetValueandTryGetValueResponse
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description | 
|---|---|
| tests/utils/_fake_python_panel_servicer.py | Added TryGetValuemethod and importedTryGetValueResponse | 
| tests/unit/test_python_panel_service_stub.py | Added tests for TryGetValueand updated exception test for GetValue | 
| tests/unit/test_panel_client.py | Renamed exception test for get_value, addedtry_get_valuetests | 
| src/nipanel/_panel_value_accessor.py | Switched from get_valuetotry_get_valueinternally | 
| src/nipanel/_panel_client.py | Refactored get_value, addedtry_get_valueand updated stubs | 
| src/ni/pythonpanel/v1/python_panel_service_pb2_grpc.pyi | Updated stub interface and documentation for TryGetValue | 
| src/ni/pythonpanel/v1/python_panel_service_pb2_grpc.py | Added TryGetValueto both stub and servicer definitions | 
| src/ni/pythonpanel/v1/python_panel_service_pb2.pyi | Introduced TryGetValueResponsetype and updated alias | 
| src/ni/pythonpanel/v1/python_panel_service_pb2.py | Updated serialized descriptor to include TryGetValueResponseand RPC | 
| protos/ni/pythonpanel/v1/python_panel_service.proto | Added TryGetValueRPC andTryGetValueResponsemessage | 
Comments suppressed due to low confidence (2)
src/nipanel/_panel_client.py:129
- The variable name try_get_value_requestinget_valueis misleading since this method uses the GetValue RPC; rename it toget_value_requestfor clarity.
        try_get_value_request = GetValueRequest(panel_id=panel_id, value_id=value_id)
src/nipanel/_panel_client.py:119
- Add a Raises:section to the docstring noting that agrpc.RpcErrorwithStatusCode.NOT_FOUNDis thrown if the value isn't set.
    def get_value(self, panel_id: str, value_id: str) -> object:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with suggestions
What does this Pull Request accomplish?
Renames
PythonPanelService.GetValue()toTryGetValue(), and add a.GetValue()that throws NOT_FOUND when the value is not found.Also updates PanelClient accordingly.
Why should this Pull Request be merged?
Since
GetValue()returns a boolean indicating success rather than throwing an exception, it would be better to call itTryGetValue(). But we also want aGetValue()that throws if the value is not found.What testing has been done?
test___no_value___get_value___raises_exceptiontest___set_value___get_value___gets_responsetest___no_value___try_get_value___gets_no_valuetest___set_value___try_get_value___gets_responsetest___get_unset_value___raises_exceptiontest___try_get_unset_value___returns_not_found