feat: add descriptor namespaces #1095

imcelroy · 2025-04-04T11:44:18Z

Public API Changes

None

Description

This will add a new namespace called descriptor for each ROS package interface generated by generate-ros-messages. The new namespace is found inside the msg|srv|action namespace for each package. For example:

namespace builtin_interfaces {
    namespace msg {
      export interface Duration {
        sec: number;
        nanosec: number;
      }
      export interface DurationConstructor {
        new(other?: Duration): Duration;
      }
      export interface Time {
        sec: number;
        nanosec: number;
      }
      export interface TimeConstructor {
        new(other?: Time): Time;
      }
      namespace descriptor {
        export interface Duration {
          sec: 'int32';
          nanosec: 'uint32';
        }
        export interface Time {
          sec: 'int32';
          nanosec: 'uint32';
        }
      }
    }
  }

The descriptor interfaces always have the format {field: "<interface_type>"}. For example:

  namespace geometry_msgs {
    namespace msg {
      namespace descriptor {
        export interface PointStamped {
          header: 'std_msgs/msg/Header';
          point: 'geometry_msgs/msg/Point';
        }
      }
    } 
  }

Here is the discussion: discussions/1091

imcelroy · 2025-04-04T11:48:50Z

Hi @minggangw, I would like to add a couple unit tests, but I'm not sure where/how you test the content of the interfaces.d.ts file? If you could point me in the right direction I'll add the unit tests and then open this for review!

coveralls · 2025-04-04T12:01:46Z

coverage: 85.325% (-0.03%) from 85.355%
when pulling 507c93d on imcelroy:add-descriptor-namespaces
into baba669 on RobotWebTools:develop.

minggangw · 2025-04-07T03:03:24Z

Hi @imcelroy thanks for submitting the PR quickly, be honest, currently, we don't have kind of unit test targeting on the generated messages for typescript specifically.

We have https://github.com/RobotWebTools/rclnodejs/blame/develop/test/types/index.test-d.ts that leverages dst to test the type definitions, I think we can add some test cases that verify the generated types under descriptor namespace, we don't need to test each, but one or two at least, how do you think?

imcelroy · 2025-04-07T07:30:24Z

Hi @minggangw, yes that sounds good to me! I will take a look and add those asap.

imcelroy · 2025-04-07T08:41:58Z

@minggangw I have added the unit tests and opened this for review, thanks for your help!

imcelroy · 2025-04-07T09:26:47Z

@minggangw It's strange, npx tsd is passing for me locally, I'm not sure why it isn't passing in the CI. I'm looking into this, but if you have any ideas let me know.

imcelroy · 2025-04-07T12:59:27Z

All seems to be good now! The Windows build that is failing seems to be present on other pull requests as well, and not directly linked to my changes. Will that block us from merging this feature?

minggangw · 2025-04-08T01:43:48Z

All seems to be good now! The Windows build that is failing seems to be present on other pull requests as well, and not directly linked to my changes. Will that block us from merging this feature?

I suspect the failure on Windows platform may be caused by env changes, because all tests do pass on ROS2 jazzy build. I will start the review work soon, thanks!

Copilot

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot · 2025-04-08T02:17:21Z

rostsd_gen/index.js

+    useSamePkg,
+    descriptorInterfaceType
+  );
+  const tmplEnd = indentString('}\n', indentlevel);


In the saveMsgAsTSD function, there is an inconsistency between the variable names 'indentlevel' and 'indentLevel'. This may lead to formatting issues; consider using a consistent variable name throughout the function.

minggangw

Thanks for submitting the PR, the implementation looks good and some comments left.

minggangw · 2025-04-08T05:08:43Z

rostsd_gen/index.js

+}
+
+function saveMsgAsTSD(rosMsgInterface, fd, descriptorInterfaceType = false) {
+  const indentlevel = descriptorInterfaceType ? 8 : 6;


nit: I prefer renaming indentlevel because it confused with indentLevel on L.292

minggangw · 2025-04-08T05:13:37Z

rostsd_gen/index.js

  typePrefix = '',
-  useSamePackageSubFolder = false
+  useSamePackageSubFolder = false,
+  descriptorInterfaceType = false


I prefer renaming descriptorInterfaceType to willGenerateDescriptionInterface

minggangw · 2025-04-08T05:20:16Z

test/types/index.test-d.ts

+);
+expectAssignable<'action_msgs/msg/GoalInfo'>(
+  cancelGoalRequestDescriptor.goal_info
+);


really like these ut 👍

minggangw · 2025-04-08T05:38:20Z

rostsd_gen/index.js

+
+      // generate descriptor msg/srv/action interfaces
+      fs.writeSync(fd, `      namespace ${descriptorInterfaceNamespace} {\n`);
+      const descriptorInterfaceType = true;


descriptorInterfaceType => willGenerateDescriptionInterface

minggangw · 2025-04-08T05:41:44Z

rostsd_gen/index.js

    isInternalServiceEventMsgInterface(rosMsgInterface);
-  saveMsgFieldsAsTSD(rosMsgInterface, fd, 8, ';', '', useSamePkg);
-  fs.writeSync(fd, '      }\n');
+  const indentLevel = descriptorInterfaceType ? 10 : 8;


Is it possible to abstract the functionality of getting the value of indent to a separate function in this file?

minggangw · 2025-04-08T06:08:26Z

You can add your name into https://github.com/RobotWebTools/rclnodejs/blob/develop/CONTRIBUTORS.md, it's up to you 😄

- rename willGenerateDescriptorInterface - create getIndentSpacing funciton - avoid similar indentLevel variable names

imcelroy · 2025-04-08T07:53:57Z

@minggangw Thank you for the review! I think I have addressed all of your comments, let me know what you think!

minggangw

LGTM!

minggangw · 2025-04-08T08:14:06Z

@imcelroy I have merged the PR, if you want to land the change in a new release asap, let me know, I can prepare for it this week.

imcelroy · 2025-04-08T08:15:57Z

@minggangw Thank you very much! Yes, if you could add this to a new release asap that would be great! I appreciate it!

This will add a new namespace called `descriptor` for each ROS package interface generated by `generate-ros-messages`. The new namespace is found inside the msg|srv|action namespace for each package. For example: ```typescript namespace builtin_interfaces { namespace msg { export interface Duration { sec: number; nanosec: number; } export interface DurationConstructor { new(other?: Duration): Duration; } export interface Time { sec: number; nanosec: number; } export interface TimeConstructor { new(other?: Time): Time; } namespace descriptor { export interface Duration { sec: 'int32'; nanosec: 'uint32'; } export interface Time { sec: 'int32'; nanosec: 'uint32'; } } } } ``` The descriptor interfaces always have the format `{field: "<interface_type>"}`. For example: ```typescript namespace geometry_msgs { namespace msg { namespace descriptor { export interface PointStamped { header: 'std_msgs/msg/Header'; point: 'geometry_msgs/msg/Point'; } } } } ``` Here is the discussion: [discussions/1091](#1091)

minggangw · 2025-04-08T10:38:12Z

@imcelroy https://www.npmjs.com/package/rclnodejs/v/0.33.0 is online 🎉

imcelroy · 2025-04-08T10:42:20Z

@minggangw wow! Thank you very much for the speed; that was impressive! 🎉

feat: add descriptor namespaces

5e5d647

fix saveActionAsTSD

c5e5cf7

imcelroy added 2 commits April 7, 2025 10:36

fix add Constructor interfaces in description namespace

903cff3

add descriptor interface tsd tests

6f32900

imcelroy marked this pull request as ready for review April 7, 2025 08:38

use expectAssignable

221138b

use Fibonacci action as example for unit test

e7e262b

minggangw requested a review from Copilot April 8, 2025 02:16

Copilot AI reviewed Apr 8, 2025

View reviewed changes

minggangw reviewed Apr 8, 2025

View reviewed changes

imcelroy added 2 commits April 8, 2025 09:51

fix review comments + linting

aff6dfc

- rename willGenerateDescriptorInterface - create getIndentSpacing funciton - avoid similar indentLevel variable names

separate contributors, add new contributions

507c93d

minggangw approved these changes Apr 8, 2025

View reviewed changes

minggangw merged commit 1b6cf74 into RobotWebTools:develop Apr 8, 2025
10 of 11 checks passed

imcelroy deleted the add-descriptor-namespaces branch April 8, 2025 08:35

feat: add descriptor namespaces #1095

feat: add descriptor namespaces #1095

Uh oh!

Conversation

imcelroy commented Apr 4, 2025

Uh oh!

imcelroy commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minggangw commented Apr 7, 2025

Uh oh!

imcelroy commented Apr 7, 2025

Uh oh!

imcelroy commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imcelroy commented Apr 7, 2025

Uh oh!

imcelroy commented Apr 7, 2025

Uh oh!

minggangw commented Apr 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw left a comment

Choose a reason for hiding this comment

Uh oh!

minggangw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

imcelroy Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

minggangw commented Apr 8, 2025

Uh oh!

imcelroy commented Apr 8, 2025

Uh oh!

minggangw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minggangw commented Apr 8, 2025

Uh oh!

imcelroy commented Apr 8, 2025

Uh oh!

minggangw commented Apr 8, 2025

Uh oh!

imcelroy commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imcelroy commented Apr 4, 2025 •

edited

Loading

coveralls commented Apr 4, 2025 •

edited

Loading

imcelroy commented Apr 7, 2025 •

edited

Loading