add list_parameters & test

[leeminju531]

This addresses the issue (#1097) by implementing the functionality for list_parameters that the node owns.
This feature should help with logging and debugging by allowing users to easily retrieve all declared parameters and their values, similar to the functionality in ROS1.

[fujitatomoya]

implementation looks good to me, probably we can add more test cases.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[Relativiteit]

@fujitatomoya Hey, I want to contribute for the first time, is there still a need for more testcases for this issue ?

[fujitatomoya]

@leeminju531 sorry to be late to get back to this. #1124 (comment) makes sense to me, could you address that? after that we can start the CI again.

[leeminju531]

It appears to be a reasonable suggestion, and I'm fully prepared to make the necessary updates in the near future.
Once these adjustments are in place, I will notify @clalancette and @fujitatomoya to request a review.

[leeminju531]

I have made revisions according to the suggestions. I kindly request @clalancette and @fujitatomoya to review the changes.

[fujitatomoya]

@clalancette your comment is addressed, can you check again?

[clalancette]

The more I look at this one, the more I'm curious why the implementation can't look like the rclcpp one. If it was more-or-less a Python port of that, it would be much easier to read (and reason about), since the logic would be the same.

@leeminju531 Can you maybe describe why the current implementation is like it is, rather than a direct port of the rclcpp code?

[leeminju531]

I referred to _list_parameters_callback, where a similar process I intend to adopt has already been implemented ;however, i understood and agree your comments.

I've observed that the service callbacks of ParameterService mostly directly call the functions owned by the node, except for _list_parameters_callback, which is implemented directly. i believe it should also referred to the function the node owns for consistency and maintainability.

I believe that the modification should be made in conjunction with your proposed comment.
@clalancette What is your opinion on this?

[clalancette]

I've observed that the service callbacks of ParameterService mostly directly call the functions owned by the node, except for _list_parameters_callback, which is implemented directly. i believe it should also referred to the function the node owns for consistency and maintainability.

I believe that the modification should be made in conjunction with your proposed comment. @clalancette What is your opinion on this?

Yes, I think that all makes a lot of sense to me. To recap:

1. Update the implementation of list_parameters in node.py so it looks similar to the rclcpp version of the same.
2. Update the implementation of _list_parameters_callback so that it calls the node.py implementation of list_parameters.

Please go ahead and do that, and I'm happy to review the result!

[leeminju531]

Please review the changes at your convenience. @clalancette

[clalancette]

Overall, this looks great. I've left one comment inline; I'm going to hold off on merging it until we figure out who is going to submit the PR to rclcpp. But I'm going to run CI on this and get it prepared for merge.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows