Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the annotation for non trivial sequence #20

Conversation

gergondet-woven
Copy link
Contributor

Before this PR, unbounded sequences of non trivial types (e.g. std_msgs/Header[]) would be annotated as Sequence[T] which is not mutable.

This would make type-checking fail on code generating such a msg, e.g.

def make_header() -> std_msgs.msg.Header: ...

my_msg = Mymsg()
for i in range(10):
    my_msg.data.append(make_header())

This fails because there's no append method on Sequence.

By making the type annotation MutableSequence the problem goes away.

Before this PR, unbounded sequences of non trivial types (e.g.
`std_msgs/Header[]`) would be annotated as `Sequence[T]` which is not
mutable.

This would make type-checking fail on code generating such a msg, e.g.

```python

def make_header() -> std_msgs.msg.Header: ...

my_msg = Mymsg()
for i in range(10):
    my_msg.data.append(make_header())
```

This fails because there's no `append` method on `Sequence`.

By making the type annotation `MutableSequence` the problem goes away.
Copy link
Member

@bonprosoft bonprosoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thank you so much for your contribution!

Comment on lines +168 to 169
"typing.MutableSequence[{}]".format(type_annotation.getter),
"typing.Sequence[{}]".format(type_annotation.setter),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly checked the rosidl_python implementation.
It seems array.array is called for instances other than array.array at the setter:

https://github.com/ros2/rosidl_python/blob/3c3cf26aaf66bfda1a0486c07cd0425aab7bff59/rosidl_generator_py/resource/_msg.py.em#L450-L453
https://github.com/ros2/rosidl_python/blob/3c3cf26aaf66bfda1a0486c07cd0425aab7bff59/rosidl_generator_py/resource/_msg.py.em#L604

so even if we pass a tuple to the setter, it will always converted it into an array.
That means, we can safely say it's a MutableSequence 👍

import array
from std_msgs.msg import Float32MultiArray

msg = Float32MultiArray()
msg.data = (1.0, 2.0, 3.0)
assert isinstance(msg.data, array.array)

...and unfortunately array module itself doesn't support type annotation (but typeshed does, so actually we can write array[float]), so MutableSequence looks good to me.

@bonprosoft bonprosoft merged commit df4bb1a into rospypi:master Jun 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants