[API Rename] spin_until_future_complete to spin_until_complete

[SteveMacenski]

Hi,

I was hoping to get some feedback on the idea of changing spin_until_future_complete to spin_until_complete. When writing application code with actions / services, I run into extending lines often while spinning to see if the server was completed. This is it of itself isn't a big deal, but I wonder if we can't shorten it a bit. I'm not sure what the "future" part of the API provides. The input is a future, but we don't have publish_message(msg) we just have publish(msg).

So I propose changing spin_until_future_complete(future) to spin_until_complete(future) which I think semantically makes more sense and also opens the opportunity in future redesigns to use other mechanics other than futures to spin under the same API without adding a few different spin_until_XYZ_complete(). This certainly isn't an earth-shattering suggestion, but if you think this would be agreeable, I'd be happy to help implement.

[wjwwood]

I don't feel strongly about it. The rationale for removing the future part makes sense to me, but I also don't feel like it is a huge drawback compared to iterating on the API. So if you want to propose a pull request that adds the new name and deprecates the old one (then ideally make sure we remove it next release), I think I'd be ok with that.

Maybe get feedback from @ivanpauno or @mabelzhang first before opening something.

[fujitatomoya]

im good to go with this. less redundancy makes sense to me.

[SteveMacenski]

OK, I'll wait for the feedback from William's other suggestions before getting into it.

I've done a quick overview of the main repos and see C++ instances of spin_until_future_complete in:

• rclcpp
• rclc

For python its

• rclpy
• ros2cli

So overall, it actually seems like it may be pretty trivial to make that conversion. I assume you'd like me to do Python at the same time?

[clalancette]

While I don't care hugely what the name is, I honestly don't think the churn here is worth it. In order to get this in, we are going to have to:

1. Add a newly named API.
2. Deprecate the old API.
3. Fix all of the usage of the deprecated name in the core, and then eventually in all downstream packages.
4. Leave both the deprecated API and the new one in place for Humble.
5. Remove the deprecated API in I-Turtle.

It just doesn't seem to be worth it to me. Maybe it will be worth it in the future if we add spin_until_XYZ_complete, but I wouldn't speculatively change it because of something that might or might not happen in the future.

But I'm not a maintainer here, this is just my 2 cents.

[SteveMacenski]

I agree its a bit of an undertaking. Though with the deprecation warning, I think it would be fixed by maintainers of downstream packages pretty quickly and without much hassle if its just a pure rename. In core, I only found it in the 4 packages listed above, since the executor models are in the client library languages, thankfully its not persistent further down in the stack than rclcpp and rclpy.

But this is why I asked before just doing it -- it is a bit of effort that I'm willing to deal with, but only if it ultimately would gain popular support to be merged. By in large though, search and replace will fix most of these issues (and a new depreciation warning on existing ones).

[wjwwood]

I understand where @clalancette is coming from, but still I think it's fine to change, especially if someone is pushing for it (like you @SteveMacenski). So unless @ivanpauno doesn't care for it, I think it's ok to do.

I do think you should do rclpy at the same time, and as @clalancette said you should do a tick-tock deprecation cycle.

[SteveMacenski]

I'd be happy to do rclpy at the same time. Lets wait on @ivanpauno's response. I'd like general consensus before I started this

[ivanpauno]

Sorry for the delay in replying.
Renaming the method sounds fine to me, but it doesn't seem to be worth the churn.

So I propose changing spin_until_future_complete(future) to spin_until_complete(future) which I think semantically makes more sense and also opens the opportunity in future redesigns to use other mechanics other than futures to spin under the same API without adding a few different spin_until_XYZ_complete()

I would rather do the renaming when that happens, though I don't mind strongly.