Skip to content

ros2param: fix parameter delete typo (use instance instead of class)#1243

Open
longway-code wants to merge 2 commits into
ros2:rollingfrom
longway-code:fix-ros2param-delete
Open

ros2param: fix parameter delete typo (use instance instead of class)#1243
longway-code wants to merge 2 commits into
ros2:rollingfrom
longway-code:fix-ros2param-delete

Conversation

@longway-code

@longway-code longway-code commented Jun 12, 2026

Copy link
Copy Markdown

ros2 param delete was assigning to the class attribute Parameter.name instead of the instance parameter.name.

While this happens to work for one-off CLI calls, it pollutes the Parameter class definition by replacing the property descriptor with a string. This causes any subsequent attempts to instantiate Parameter messages in the same process to crash with AttributeError.

This PR fixes the typo and adds a basic integration test (test_verb_delete.py).

@longway-code longway-code marked this pull request as ready for review June 12, 2026 15:56
@longway-code longway-code changed the title ros2param: fix param delete failure (use instance instead of class) ros2param: fix parameter delete typo (use instance instead of class) Jun 12, 2026
…ment

The delete parameter command was assigning to the class attribute Parameter.name instead of the instance attribute parameter.name, causing delete requests to be sent without the target parameter name. Also added test_verb_delete.py to test the delete command.

Signed-off-by: Longwei Liu <longwei.llw@gmail.com>
@longway-code longway-code force-pushed the fix-ros2param-delete branch from bf8194c to 4552930 Compare June 12, 2026 16:30

@fujitatomoya fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm.

this is real bug which has been several years, thanks for fixing this.

@fujitatomoya

Copy link
Copy Markdown
Collaborator

Pulls: #1243
Gist: https://gist.githubusercontent.com/fujitatomoya/5a4420014c3c431a2fe197d3b1d21191/raw/c023bd1f8b21a6b198de72acf9e5985a5c87c1bd/ros2.repos
BUILD args: --packages-above-and-dependencies ros2param
TEST args: --packages-above ros2param
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19525

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya

Copy link
Copy Markdown
Collaborator

Signed-off-by: Longwei Liu <longwei.llw@gmail.com>
@longway-code

Copy link
Copy Markdown
Author

@longway-code can you address https://ci.ros2.org/job/ci_linux/29374/testReport/junit/ros2param.ros2param.test/test_flake8/test_flake8/?

Thanks for the review! I have wrapped the long comment lines, The flake8 linting errors should be resolved now.

@fujitatomoya

Copy link
Copy Markdown
Collaborator
  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

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.

2 participants