Add type hints to tf2_ros_py code(#275)

[surfertas]

1. Added type hints to member functions in tf2_ros_py.
2. Removed redundant :rtypes annotations from docstring.
3. Used TypeVar() to define types for LookupTransformGoal and LookupTransformResult.

[surfertas]

@clalancette Hi, can you take a look at the PR for adding to type hints to buffer_client.py. If all good, I will convert the other files in tf2_ros_py and create a commit for each file.

[clalancette]

This looks generally good to me, so I'd encourage you to continue on and do the rest of the files as well. I wouldn't bother doing a separate commit per-file; we are probably going to squash-merge it at the end anyway. Thanks for the work so far!

[surfertas]

@clalancette @sloretz This is ready for review.
Please note in buffer_interface.py, I have handled object_stamped accepting PyKDL objects and stamped messages by defining a TransformableObject type.

geometry2/tf2_ros_py/tf2_ros/buffer_interface.py

Line 49 in cab7c8c

def transform(self, object_stamped, target_frame, timeout=Duration(), new_type = None):

Separately:

1. The constructor for StaticTransformBroadcaster and TransformListener both have qos as an Optional[Union[QoSProfile, int]] type and handle None while TransformBroadcaster has qos not as an Optional and does not handle the None case and does not raise a type error if None is passed inadvertently. Should this be handled?

2. Should time be changed to time.to_msg() constistent with lookup_transform_full() and the signature?
   

   geometry2/tf2_ros_py/tf2_ros/buffer_client.py

   Line 83 in cab7c8c

   goal.source_time = time

If yes, I will create a new PR for 1 & 2 and fix the tests if required.

[clalancette]

@surfertas Thanks for the work so far. This PR a little hard to review at the moment because it contains changes from other PRs as well. If you rebase this onto the latest ros2 branch, it will have just the changes from you. Then I'll take a look at answering your other questions. Thanks.

[surfertas]

@clalancette Apologies and no problem. You should now only see the changes from me. Thank you.

[clalancette]

I've got a lot of comments inline, but they are all easy to fix and most of them are of the same type. So it should be pretty easy to fix them.

To comment on your other questions:

The constructor for StaticTransformBroadcaster and TransformListener both have qos as an Optional[Union[QoSProfile, int]] type and handle None while TransformBroadcaster has qos not as an Optional and does not handle the None case and does not raise a type error if None is passed inadvertently. Should this be handled?

Yeah, it should be handled. I'd change the TransformBroadcaster to take a None by default, and then have the code construct the proper QoS object if it is None. That makes it similar to the other two.

Should time be changed to time.to_msg() constistent with lookup_transform_full() and the signature?

Sorry, I don't understand this question. Can you expand on it a little more?

[clalancette]

Overall, I think this looks great. Thanks for taking this on @surfertas . There are 3 open items here:

1. We still don't have a resolution on kwargs for set_transform.
2. We should fix TransformBroadcaster to have a None qos by default, and construct the object inside of the method (like we do with StaticTransformBroadcaster and TransformListener.
3. We need to fix up lookup_transform to take the right type in.

However, all of these can be left as follow-up tasks. @surfertas could you please open up issues for all 3 of the above items? Once that is done, I'm happy to approve and run CI on this.

[surfertas]

@clalancette three separate issues created.

Separately, are there any plans to integrate stubs? Ultimately, will allow users of the library to type check their code.

[clalancette]

@clalancette three separate issues created.

Fantastic, thanks.

Separately, are there any plans to integrate stubs? Ultimately, will allow users of the library to type check their code.

I don't think we've considered it.

[clalancette]

Looks good to me, thanks for all of the work here. I'll run CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

@surfertas Tests are failing, probably because you need a <test_depend>sensor_msgs</test_depend> in the package.xml

[surfertas]

@surfertas Tests are failing, probably because you need a <test_depend>sensor_msgs</test_depend> in the package.xml

@clalancette Has been added.

[clalancette]

@clalancette Has been added.

Thanks. Here's another CI try:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

Looks happy, I'm going to go ahead and merge this. Thanks again for the contribution.