-
Notifications
You must be signed in to change notification settings - Fork 79
Add some missing methods from rcl #1106
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
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
This PR adds several missing count methods and corresponding tests in both the JavaScript and C++ bindings for rcl. The key changes include adding publisher and subscription count methods, node client/service count methods, and a context domain ID getter.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-subscription.js | Adds test for the new publisherCount property on subscriptions. |
| test/test-publisher.js | Adds test for the new subscriptionCount property on publishers. |
| test/test-node.js | Adds tests for node.countClients and node.countServices methods. |
| test/test-context.js | Adds test for verifying the context domainId getter. |
| src/rcl_subscription_bindings.cpp | Introduces the C++ binding for getPublisherCount. |
| src/rcl_publisher_bindings.cpp | Introduces the C++ binding for getSubscriptionCount. |
| src/rcl_node_bindings.cpp | Introduces C++ bindings for countPublishers, countSubscribers, countClients, and countServices. |
| src/rcl_graph_bindings.cpp | Removes duplicate count methods already provided by node bindings. |
| src/rcl_context_bindings.cpp | Introduces the C++ binding for getDomainId. |
| lib/subscription.js | Adds publisherCount getter to the Subscription class. |
| lib/publisher.js | Adds subscriptionCount getter to the Publisher class. |
| lib/node.js | Adds countClients and countServices methods to the Node class. |
| lib/context.js | Adds domainId getter to the Context class. |
Comments suppressed due to low confidence (3)
test/test-subscription.js:33
- [nitpick] Using 'String' as a variable name can be misleading, as it conflicts with the built-in String type. Consider renaming it to 'messageType' or a similar descriptive identifier.
const String = 'std_msgs/msg/String';
lib/publisher.js:77
- The doc comment should use the plural 'subscriptions' instead of 'subscription'.
* Get the number of subscription to this publisher.
lib/node.js:1074
- The comment for countServices appears to be incorrect; update it to reference 'services' rather than 'client'.
* Get the number of client on a given service name.
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
This PR adds missing methods to the rclnodejs bindings to expose functionalities for counting publishers, subscriptions, clients, services, and retrieving the context’s domain ID.
- Introduces new native bindings in the rcl_subscription, rcl_publisher, rcl_node, and rcl_context modules.
- Adds corresponding JavaScript getters and test cases to verify the new methods.
- Removes duplicate count methods from the rcl_graph module to streamline the API.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-subscription.js | Adds a test for the subscription's publisher count getter. |
| test/test-publisher.js | Adds a test for the publisher's subscription count getter. |
| test/test-node.js | Adds tests for counting clients and services on a node. |
| test/test-context.js | Adds a test for getting the context domain ID. |
| src/rcl_subscription_bindings.cpp | Introduces GetPublisherCount binding for subscriptions. |
| src/rcl_publisher_bindings.cpp | Introduces GetSubscriptionCount binding for publishers. |
| src/rcl_node_bindings.cpp | Introduces CountPublishers, CountSubscribers, CountClients, and CountServices bindings. |
| src/rcl_graph_bindings.cpp | Removes duplicate count methods. |
| src/rcl_context_bindings.cpp | Introduces GetDomainId binding. |
| lib/subscription.js | Adds a publisherCount getter that calls the new native method. |
| lib/publisher.js | Adds a subscriptionCount getter with updated documentation. |
| lib/node.js | Adds countClients and countServices methods. |
| lib/context.js | Adds a domainId getter that wraps the new native getDomainId method. |
Comments suppressed due to low confidence (1)
test/test-subscription.js:33
- [nitpick] Using 'String' as a variable name may be confused with the native String type; consider renaming it to something like 'msgType'.
const String = 'std_msgs/msg/String';
lib/publisher.js
Outdated
|
|
||
| /** | ||
| * Get the number of subscriptions to this publisher. | ||
| * @returns {number} The number of subscription |
Copilot
AI
Apr 29, 2025
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.
The return description should use the plural 'subscriptions' for grammatical consistency.
| * @returns {number} The number of subscription | |
| * @returns {number} The number of subscriptions |
test/test-publisher.js
Outdated
| rclnodejs.spin(node); | ||
| }); | ||
|
|
||
| it('Test count of subscription', function () { |
Copilot
AI
Apr 29, 2025
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.
[nitpick] Consider renaming the test to 'Test count of subscriptions' for grammatical consistency.
| it('Test count of subscription', function () { | |
| it('Test count of subscriptions', function () { |
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
This PR adds missing methods to count publishers, subscriptions, clients, and services in rclnodejs, along with corresponding tests and binding implementations.
- New tests have been implemented to verify the newly added methods.
- Binding functions in C++ for subscriptions, publishers, nodes, and contexts have been extended.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-subscription.js | Added test to verify publisher count on a subscription. |
| test/test-publisher.js | Added test to verify subscription count on a publisher. |
| test/test-node.js | Added tests for counting clients and services on a node. |
| test/test-context.js | Added test to verify the context's domain ID. |
| src/rcl_subscription_bindings.cpp | Added GetPublisherCount binding. |
| src/rcl_publisher_bindings.cpp | Added GetSubscriptionCount binding. |
| src/rcl_node_bindings.cpp | Added countPublishers, countSubscribers, countClients, and countServices bindings. |
| src/rcl_graph_bindings.cpp | Removed redundant count functions. |
| src/rcl_context_bindings.cpp | Added GetDomainId binding. |
| lib/subscription.js | Added publisherCount getter. |
| lib/publisher.js | Added subscriptionCount getter. |
| lib/node.js | Added countClients and countServices methods. |
| lib/context.js | Added domainId getter. |
Comments suppressed due to low confidence (2)
src/rcl_node_bindings.cpp:330
- The order of arguments in the THROW_ERROR_IF_NOT_EQUAL macro is reversed here compared to similar bindings. The actual return value should be the first argument and RCL_RET_OK the second.
THROW_ERROR_IF_NOT_EQUAL( RCL_RET_OK, rcl_count_publishers(node, topic_name.c_str(), &count),
src/rcl_node_bindings.cpp:345
- The parameter order in the THROW_ERROR_IF_NOT_EQUAL macro call is reversed compared to other similar binding functions. Swap the order so that the actual return value is the first argument.
THROW_ERROR_IF_NOT_EQUAL( RCL_RET_OK, rcl_count_subscribers(node, topic_name.c_str(), &count),
This PR adds missing methods to count publishers, subscriptions, clients, and services in rclnodejs, along with corresponding tests and binding implementations. - New tests have been implemented to verify the newly added methods. - Binding functions in C++ for subscriptions, publishers, nodes, and contexts have been extended. | File | Description | | ----------------------------------- | ---------------------------------------------------- | | test/test-subscription.js | Added test to verify publisher count on a subscription. | | test/test-publisher.js | Added test to verify subscription count on a publisher. | | test/test-node.js | Added tests for counting clients and services on a node. | | test/test-context.js | Added test to verify the context's domain ID. | | src/rcl_subscription_bindings.cpp | Added GetPublisherCount binding. | | src/rcl_publisher_bindings.cpp | Added GetSubscriptionCount binding. | | src/rcl_node_bindings.cpp | Added countPublishers, countSubscribers, countClients, and countServices bindings. | | src/rcl_graph_bindings.cpp | Removed redundant count functions. | | src/rcl_context_bindings.cpp | Added GetDomainId binding. | | lib/subscription.js | Added publisherCount getter. | | lib/publisher.js | Added subscriptionCount getter. | | lib/node.js | Added countClients and countServices methods. | | lib/context.js | Added domainId getter. | Fix: #1107
This PR adds missing methods to count publishers, subscriptions, clients, and services in rclnodejs, along with corresponding tests and binding implementations.
Fix: #1107