Add GetNumEntities() for action client #1123
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new function, GetNumEntities(), for the action client and includes corresponding tests to verify its behavior. Key changes include:
- Adding a new type assertion in tests for the getNumEntities call.
- Implementing the GetNumEntities function in the C++ action client bindings.
- Updating the JavaScript API to expose getNumEntities on the ActionClient class.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added type assertion for getNumEntities |
| test/test-action-client.js | Added a test validating the expected values returned by getNumEntities |
| src/rcl_action_server_bindings.cpp | Removed unused ActionSendCancelRequest function from the action server bindings |
| src/rcl_action_client_bindings.cpp | Added GetNumEntities implementation and updated the exports with getNumEntities |
| lib/action/client.js | Added a getNumEntities method in the ActionClient class, calling the native binding |
Comments suppressed due to low confidence (1)
test/types/index.test-d.ts:348
- [nitpick] Consider asserting the specific structure of the object returned by getNumEntities (e.g., checking keys like subscriptionsNumber, timersNumber, etc.) to improve test precision.
expectType<object>(actionClient.getNumEntities());
src/rcl_action_client_bindings.cpp
Outdated
| std::string error_text{ | ||
| "Failed to get number of entities for 'rcl_action_client_t'"}; | ||
| Napi::Error::New(env, error_text).ThrowAsJavaScriptException(); | ||
| throw; |
There was a problem hiding this comment.
[nitpick] After calling Napi::Error::New(...).ThrowAsJavaScriptException(), the subsequent 'throw;' may be redundant and could be removed to rely solely on the JavaScript exception mechanism.
| throw; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new API method GetNumEntities() to the action client to retrieve the count of various wait-set-related entities. Key changes include:
- Adding type assertions in tests for the new API.
- Implementing GetNumEntities in the native bindings and exposing it in the JS client.
- Updating tests to verify the entity counts.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added type assertions for the getNumEntities return object. |
| test/test-action-client.js | Introduced tests verifying expected entity counts. |
| src/rcl_action_server_bindings.cpp | Removed redundant ActionSendCancelRequest and added ActionTakeCancelRequest. |
| src/rcl_action_client_bindings.cpp | Added GetNumEntities implementation and updated exports accordingly. |
| lib/action/client.js | Added the getNumEntities method on the ActionClient class. |
lib/action/client.js
Outdated
|
|
||
| /** | ||
| * Get the number of wait set entities that make up an action entity. | ||
| * @return {object} - The number of subscriptions, guard_conditions, timers, and clients and services. |
There was a problem hiding this comment.
[nitpick] Consider expanding the doc comment for getNumEntities() to explicitly list the returned object properties (subscriptionsNumber, guardConditionsNumber, timersNumber, clientsNumber, servicesNumber) for better clarity.
| * @return {object} - The number of subscriptions, guard_conditions, timers, and clients and services. | |
| * @return {object} - An object containing the number of various entities. | |
| * @property {number} subscriptionsNumber - The number of subscriptions. | |
| * @property {number} guardConditionsNumber - The number of guard conditions. | |
| * @property {number} timersNumber - The number of timers. | |
| * @property {number} clientsNumber - The number of clients. | |
| * @property {number} servicesNumber - The number of services. |
Copilot uses AI. Check for mistakes.
| return env.Undefined(); | ||
| } | ||
|
|
||
| Napi::Value GetNumEntities(const Napi::CallbackInfo& info) { |
There was a problem hiding this comment.
[nitpick] Add a comment outlining the mapping between native entity counts and the JavaScript object keys to improve maintainability and assist future developers in understanding the conversion.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new method, GetNumEntities(), for the action client to retrieve the counts of various wait set entities. It adds corresponding tests and bindings in both the native C++ layer and the JavaScript API, and updates the documentation accordingly.
- Introduces GetNumEntities() in rcl_action_client_bindings.cpp and exposes it as getNumEntities() in the ActionClient class.
- Updates tests in both TypeScript and JavaScript to verify the returned counts.
- Removes duplicate and unused cancel request functions from client and server bindings.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Added a type test for the getNumEntities() method. |
| test/test-action-client.js | Added a functional test verifying returned entity counts. |
| src/rcl_action_server_bindings.cpp | Added a new binding for taking cancel requests. |
| src/rcl_action_client_bindings.cpp | Introduced GetNumEntities() and adjusted export order. |
| lib/action/client.js | Added JSDoc documentation and a getNumEntities() method. |
| rcl_reset_error(); | ||
| std::string error_text{ | ||
| "Failed to get number of entities for 'rcl_action_client_t'"}; |
There was a problem hiding this comment.
Consider including the actual error code value in the thrown error message to provide better context for debugging failures from rcl_action_client_wait_set_get_num_entities.
| rcl_reset_error(); | |
| std::string error_text{ | |
| "Failed to get number of entities for 'rcl_action_client_t'"}; | |
| std::string error_text = "Failed to get number of entities for 'rcl_action_client_t'. Error code: " + std::to_string(ret); | |
| rcl_reset_error(); |
Copilot uses AI. Check for mistakes.
2 participants
This PR introduces a new method, GetNumEntities(), for the action client to retrieve the counts of various wait set entities. It adds corresponding tests and bindings in both the native C++ layer and the JavaScript API, and updates the documentation accordingly.
Fix: #1122