Clap demo

[avanmalleghem]

Related to issue : #12

I tested it on Jazzy. Here is an example of working commands :

ros2 run examples_rclrs_clap_demo clap_demo --count 15 --ros-args -r topic:=new_topic -- --name John

[esteve]

@avanmalleghem so cool, thanks! I can't speak for @mxgrey, but the way I understood the ticket was to integrate clap into some of the existing examples. While useful to show how clap can be used with rclrs, I'd prefer if it weren't a separate package that we'd have to maintain.

[avanmalleghem]

@avanmalleghem so cool, thanks! I can't speak for @mxgrey, but the way I understood the ticket was to integrate clap into some of the existing examples. While useful to show how clap can be used with rclrs, I'd prefer if it weren't a separate package that we'd have to maintain.

Ok I let some time to @mxgrey so that we have his point of view aswell and I modify it if needed :) Thanks for your feedback !

[esteve]

@avanmalleghem what I meant is that given that the changes between this and the existing example are very minimal, I prefer not to maintain two packages that are almost identical. Could you integrate these changes into one of the existing examples? Thanks

[avanmalleghem]

Fair point ! Will do it asap (max this weekend)

[esteve]

@avanmalleghem there's no rush 🙂 Thanks again for all the work!

[avanmalleghem]

@esteve ok for you if I include clap into the rust_pubsub/src/simple_publisher.rs ?

[esteve]

@avanmalleghem I'd do it in https://github.com/ros2-rust/examples/tree/main/rclrs/minimal_pub_sub, as it's the most commonly used example. For example in the publisher and add a counter like you did in this PR. Could you make it so that if no arguments are passed, the example behaves like now? Thanks

[luca-della-vedova]

Is it possible to make this an Option<u32> and, if None, we don't limit the number of messages instead of publishing u32::MAX messages?

[avanmalleghem]

@luca-della-vedova I changed it, thanks for the suggestion. I'm not sure about the "map_or" for the condition. Is it the best way to handle it ?

[luca-della-vedova]

Ah interesting! I just found out that since you are just returning true when the option is None for your use case you can just change:

args.count.map_or(true, |count| publish_count <= count)
// Into
args.count.is_none_or(|count| publish_count <= count)

Then it's a bit more readable

[luca-della-vedova]

Usually dependencies are only so specific if they need a fix that is present in the requested minor / patch version. Does your example need anything specific from 4.5.51 or is 4.5, or even 4 good enough? If so can you just change it to the less strict requirement?

[avanmalleghem]

I guess version 4 is good enough :) I fixed it ! thanks for the best practice

[luca-della-vedova]

I gave it a try and it works well, can always bikeshed on comments etc. but code wise LGTM