Add explicit LYRICAL distro identifier (2605)#1488
Add explicit LYRICAL distro identifier (2605)#1488minggangw merged 2 commits intoRobotWebTools:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Registers the upcoming ROS 2 Lyrical Luth distro in rclnodejs’ distro utilities so ROS_DISTRO=lyrical resolves to a concrete distro ID instead of the FUTURE sentinel.
Changes:
- Added
DistroId.LYRICAL = 2605and mapped the'lyrical'name inDistroNameIdMap. - Updated the distro util test to expect 9 known distro names (was 8).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/distro.js |
Adds the LYRICAL distro ID and name→ID mapping for ROS_DISTRO=lyrical. |
test/test-distro.js |
Updates the expected count of known distro names to include Lyrical. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IRON: 2305, | ||
| JAZZY: 2405, | ||
| KILTED: 2505, | ||
| LYRICAL: 2605, | ||
| ROLLING: 5000, | ||
| FUTURE: 9999, // unrecognized ROS_DISTRO assumed newer than Rolling |
There was a problem hiding this comment.
Adding LYRICAL: 2605 changes getDistroId('lyrical') from the FUTURE sentinel (9999) to 2605, which will make all >= DistroId.ROLLING feature gates evaluate false on Lyrical. There are several such gates (e.g., lib/node.js:1590/1624, lib/subscription.js:154, lib/action/client.js:102), so Lyrical will now be treated as not supporting those Rolling-gated features. If Lyrical is expected to include those APIs (as a stable release branched from Rolling), update those checks to include DistroId.LYRICAL (or introduce a helper like “isRollingOrNewerStable()” / “isAtLeastDistroName()”) so behavior doesn’t regress compared to the previous FUTURE fallback.
| assert.equal( | ||
| distroNames.length, | ||
| 8, | ||
| 9, | ||
| 'Incorrect number of known distro names' | ||
| ); |
There was a problem hiding this comment.
The test asserts an exact getKnownDistroNames() length, which makes it fail every time a new distro is added (as in this PR) without adding much additional validation. Consider instead asserting that the array contains the expected distro names (including lyrical) and that each entry round-trips via getDistroId()/getDistroName(), without locking the total count to a specific number.
|
coverage: 85.864% (+0.002%) from 85.862% — minggangw:fix-1458-4 into RobotWebTools:develop |
Register the upcoming ROS 2 Lyrical Luth (May 2026) distribution so that distro-gated code paths activate correctly when
ROS_DISTRO=lyricalis set, rather than falling through to theFUTUREsentinel.Changes
lib/distro.jsLYRICAL: 2605toDistroIdenum andDistroNameIdMaptest/test-distro.jsexpectedDistrosarray that verifies each known distro (includinglyrical) is present viaincludes()Why 2605? Follows the existing
YYMMconvention — Lyrical targets May 2026 GA.Fix: #1458