Support hyphenated ROSIDL subdirs#1390
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for ROSIDL packages with hyphenated subdirectory names (e.g., msg-common, msg-ros2) by updating the subfolder extraction logic to normalize these to their base interface types (msg, srv, action).
Changes:
- Modified
getSubFolder()to extract base interface type from hyphenated subdirectory names usingsplit('-')[0] - Added comprehensive test case using
mrpt_msgspackage to validate hyphenated subfolder support - Refactored test setup in
test-native-loader.jsto avoid repeated module reloading - Updated CI workflows to install
mrpt-msgspackage for testing - Simplified process exit handling in test runner script
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 |
|---|---|
| rosidl_gen/packages.js | Updated regex pattern and added logic to normalize hyphenated subfolder names to base interface types |
| test/test-rosidl-message-generator.js | Added test case for mrpt_msgs with hyphenated subfolders to verify message loading and field access |
| test/test-native-loader.js | Refactored module loading to cache native_loader.js and reuse TestHelpers between tests |
| scripts/run_test.js | Simplified exit code handling by directly setting process.exitCode and calling process.exit() |
| .github/workflows/linux-x64-build-and-test.yml | Added mrpt-msgs package installation for CI testing |
| .github/workflows/linux-arm64-build-and-test.yml | Added mrpt-msgs package installation for CI testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| // If the |amentExecuted| equals to false, the file's extension will be assigned as |
There was a problem hiding this comment.
When the regex match fails (match is null or undefined), the function will fall through to line 64 which returns the file extension. However, if amentExecuted is true and the match fails, this might indicate a malformed path that should be handled differently. Consider adding an explicit else clause or a fallback return statement within the amentExecuted block to make the error handling more explicit, or add a comment explaining that falling through to line 64 is intentional for this case.
| } | |
| // If the |amentExecuted| equals to false, the file's extension will be assigned as | |
| // If |amentExecuted| is true but the expected pattern is not found, | |
| // fall back to using the file's extension as the subfolder name. | |
| return path.parse(filePath).ext.substr(1); | |
| } | |
| // If |amentExecuted| equals to false, the file's extension will be assigned as |
This PR adds support for ROSIDL packages with hyphenated subdirectory names (e.g., `msg-common`, `msg-ros2`) by updating the subfolder extraction logic to normalize these to their base interface types (`msg`, `srv`, `action`).
**Changes:**
- Modified `getSubFolder()` to extract base interface type from hyphenated subdirectory names using `split('-')[0]`
- Added comprehensive test case using `mrpt_msgs` package to validate hyphenated subfolder support
- Refactored test setup in `test-native-loader.js` to avoid repeated module reloading
- Updated CI workflows to install `mrpt-msgs` package for testing
- Simplified process exit handling in test runner script
Fix: #1388
2 participants
This PR adds support for ROSIDL packages with hyphenated subdirectory names (e.g.,
msg-common,msg-ros2) by updating the subfolder extraction logic to normalize these to their base interface types (msg,srv,action).Changes:
getSubFolder()to extract base interface type from hyphenated subdirectory names usingsplit('-')[0]mrpt_msgspackage to validate hyphenated subfolder supporttest-native-loader.jsto avoid repeated module reloadingmrpt-msgspackage for testingFix: #1388