-
Notifications
You must be signed in to change notification settings - Fork 79
Implement event handler #1159
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
Implement event handler #1159
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 support for event handler functionality for subscriptions and publishers by extending native bindings, updating the executor and handle manager, and augmenting test coverage.
- Adds C++ bindings for event handle management.
- Updates Node, Subscription, and Publisher classes to integrate event callbacks.
- Enhances tests and type definitions to cover new event functionality.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test-d.ts | Adds test cases for event callbacks on subscriptions and publishers. |
| test/test-type-description-service.js | Minor update to delay tests based on distro conditions. |
| test/test-event-handle.js | Introduces comprehensive tests for subscription and publisher events. |
| src/rcl_event_handle_bindings.{h,cpp} | Implements new bindings for event handle functionality. |
| src/handle_manager.{h,cpp} | Integrates event handles into wait set management and entity tracking. |
| src/executor.cpp | Updates wait set resizing and ready handle collection to include events. |
| src/addon.cpp | Registers the new event handle bindings. |
| lib/{subscription.js, publisher.js, node.js} | Extends API methods to support event callbacks on subscriptions/publishers. |
| lib/lifecycle_publisher.js | Updates publisher creation to work with node handles. |
| binding.gyp | Adds the event handle bindings source file. |
Comments suppressed due to low confidence (2)
lib/node.js:620
- Consider using a more descriptive error message in createPublisher when eventCallbacks is provided but is not an instance of PublisherEventCallbacks, for example: 'Invalid argument: eventCallbacks must be an instance of PublisherEventCallbacks.'
if (typeof typeClass !== 'function' || typeof topic !== 'string' || (eventCallbacks && !(eventCallbacks instanceof PublisherEventCallbacks))) {
lib/node.js:688
- Use a more detailed error message in createSubscription when eventCallbacks is provided but not an instance of SubscriptionEventCallbacks (e.g., 'Invalid argument: eventCallbacks must be an instance of SubscriptionEventCallbacks.') to improve debugging clarity.
if (typeof typeClass !== 'function' || typeof topic !== 'string' || (eventCallbacks && !(eventCallbacks instanceof SubscriptionEventCallbacks))) {
This PR adds support for subscription and publisher event handlers across native bindings and the JavaScript API, and updates tests and type definitions accordingly. - Introduce C++ bindings and event handle management in `HandleManager` and `Executor` - Extend `Node`, `Subscription`, and `Publisher` classes to accept and clean up event callbacks - Add and adjust tests and type declarations for event handler functionality Fix: #1142, #492
This PR adds support for subscription and publisher event handlers across native bindings and the JavaScript API, and updates tests and type definitions accordingly.
HandleManagerandExecutorNode,Subscription, andPublisherclasses to accept and clean up event callbacksFix: #1142, #492