ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
289 stars 224 forks source link

Add missing `Optional` types to `node.py` on `humble` branch #1306

Closed alberthli closed 3 months ago

alberthli commented 3 months ago

Added Optional types to function declarations in node.py where relevant on the humble branch. Missing Optional types cause static type checking failures. Example:

class ExampleNode(Node):
    def __init__(self, cbg: Optional[CallbackGroup] = None) -> None:
        super().__init__("example_node")
        self.timer = self.create_timer(
            0.001,
            self.timer_callback,
            callback_group=cbg,  # fails static type checking - this field wasn't annotated as Optional[CallbackGroup]
        )
Barry-Xu-2018 commented 3 months ago

LGTM. The rolling version already operates in this way.

alberthli commented 3 months ago

Great. Are there any next steps before merging?

Barry-Xu-2018 commented 3 months ago

The next step is to pass the CI since this PR modifies the code.
Please wait for the CI test results.

clalancette commented 3 months ago

although this fixes static type checking, i guess we would want to proceed these changes from the upstream and then backport to downstream to avoid those fixes are scattered over distros. could you address those issues in rolling or pick up merged PR to be backported to released distros?

It looks like these are already fixed on the rolling branch, so we are OK there.

In terms of backporting, I'm actually of the opinion that we should take this PR as-is. While it would be nice to cherry-pick changes from the upstream branches, I think it will be just as much work as doing them one-by-one here. And since @alberthli already has this one queued up and ready to go, I think we should just take it (though obviously we have to re-run CI here to make sure it is passing).

fujitatomoya commented 3 months ago

thanks, and im okay. sometimes backporting is not straight forward either so would require some adjustment.

clalancette commented 3 months ago

CI looks good here, so going ahead and merging this one in. Thanks @alberthli !