Skip to content

Conversation

@mahmoud-ghalayini
Copy link
Collaborator

Public API Changes
Added ParameterClient class for remote parameter access:

  • node.createParameterClient(remoteNodeName, options)
  • node.destroyParameterClient(parameterClient)
  • ParameterClient with full API for get/set/list/describe operations
  • Exported parameterTypeFromValue utility

#1317

@mahmoud-ghalayini mahmoud-ghalayini force-pushed the feat-add-parameter-client-for-external-parameter-access-1317 branch from 05728bf to 9cb7abb Compare October 31, 2025 13:31
@coveralls
Copy link

coveralls commented Oct 31, 2025

Coverage Status

coverage: 82.737% (+0.7%) from 82.083%
when pulling 14ed2c5 on mahmoud-ghalayini:feat-add-parameter-client-for-external-parameter-access-1317
into 7c306aa on RobotWebTools:develop.

Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting the PR, will take a look later.

Copy link
Member

@minggangw minggangw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR with effective unit test & ts declaration, LGTM!

debug(`Creating client for service: ${serviceName}`);

const client = this.#node.createClient(serviceInterface, serviceName);
this.#clients.set(serviceType, client);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be serviceName? So we can align with L.329 in destroy() function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, I think it should be the other way around we should change serviceName to serviceType since when we are getting or creating the client we use serviceType (GetParameter/SetParameter)

}

this.#node = node;
this.#remoteNodeName = this.#normalizeNodeName(remoteNodeName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is it necessary to validate the node name by

validateNodeName(name) {

before normalizing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to accept both turtlesim and /turtlesim then we should normalize then validate

@mahmoud-ghalayini mahmoud-ghalayini force-pushed the feat-add-parameter-client-for-external-parameter-access-1317 branch from 9cb7abb to 14ed2c5 Compare November 3, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants