Open ivanpauno opened 5 years ago
Further explanation:
After ros2/rosidl_python#60, print(feedback_msg.feedback)
looks nice, but print(feedback_msg.feedback.partial_sequence)
doesn't. Example here.
Maybe, we should override array.array
__repr__
method, instead of modifying the __repr__
of the message.
Maybe, we should override array.array repr method
That would affect any code using the Python type array.array
. I would consider this highly undesired.
Maybe, we should override array.array repr method
That would affect any code using the Python type
array.array
. I would consider this highly undesired.
Yes, my idea was subclass from array.array
and override the __repr__
method, instead of using array.array
directly. I think that would be ok.
my idea was subclass from array.array
I don't think we should introduce a custom array class either. A message field should store a vanilla array.array
.
my idea was subclass from array.array
I don't think we should introduce a custom array class either. A message field should store a vanilla
array.array
.
Ok. feedback.partial_sequence
is of type array.array
, so considering your last comment we should close this.
Ok.
feedback.partial_sequence
is of typearray.array
, so considering your last comment we should close this.
No, I don't think so. We just need to do the same thing here that we did for messages; that is, we override __repr__
for our class, munging the output from array.array
so that it looks nicer. You can see that here: https://github.com/ros2/rosidl_python/blob/master/rosidl_generator_py/resource/_msg.py.em#L333 . We just need to do the same for actions and services, which I forgot at the time.
You can see that here: https://github.com/ros2/rosidl_python/blob/master/rosidl_generator_py/resource/_msg.py.em#L333
@clalancette Please use permalinks since this doesn't point anymore where you likely intended it to point to.
I guess you wanted to point to the repr method of a generated message?
We just need to do the same for actions and services, which I forgot at the time.
@clalancette The specific sub elements of a service - the request and the response - are just messages themselves. So I don't understand where you think those would need similar changes?
.. looks nice, but print(feedback_msg.feedback.partial_sequence) doesn't Ok.
feedback.partial_sequence
is of typearray.array
, so considering your last comment we should close this.
@ivanpauno I agree. I will go ahead and do so. Please feel free to continue commenting and this can be reopened if necessary.
@clalancette The specific sub elements of a service - the request and the response - are just messages themselves. So I don't understand where you think those would need similar changes?
So I may be wrong about services, but the problem that @ivanpauno first reported in the description is still a problem. Running the action_tutorials_py
client (on master as of today) still shows array
:
$ ros2 run action_tutorials_py fibonacci_action_client
[INFO] [fibonacci_action_client]: Goal accepted :)
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5, 8])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5, 8, 13])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5, 8, 13, 21])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5, 8, 13, 21, 34])
[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55])
[INFO] [fibonacci_action_client]: Result: array('i', [0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55])
So I think this is still a problem (maybe just in actions). Thus I'll reopen.
One possible solution is to first create an array.array object, and then set to that object a custom __repr__
method. That wouldn't affect others array.array
instances, and wouldn't introduce a custom class. Something like:
x = array.array(typecode)
x.__repr__ = custom_repr
I don't like much the idea of "hacking" a method without changing the type of the object, but it's a possible solution if we don't want to create a custom class.
I took another quick look at this.
https://github.com/ros2/demos/tree/master/action_tutorials/action_tutorials_interfaces does indeed invoke _msg.py.em
to generate the code (the result ends up in /install/action_tutorials_interfaces/lib/python3.6/site-packages/action_tutorials_interfaces/action/_fibonacci.py
). The problem is that what action_tutorials_py is printing out is not the Fibonacci_Feedback class itself, but rather the partial_sequence
member, which is indeed of type array.array
.
Thus, while we could override the __repr__
method of that class when we return it from the decorator, I don't really think we should. That user will want to do standard things with that member, so it should be a standard type. Thus, I think the thing to do here is to fix up https://github.com/ros2/demos/blob/97651912ebad439a6bc62c34671cad6a5bfee785/action_tutorials/action_tutorials_py/action_tutorials_py/fibonacci_action_client.py#L61 so that it strips off the array.array
; after all, it is client code at that point, so it is a good example.
@ivanpauno If you agree with this, then I'll move this issue over to https://github.com/ros2/demos, and we can fix it over there. Thoughts?
Yes, I agree that's the best solution.
The only bad thing is that you have to repeat the stripping in every demo with a similar use case, but that's how array.array
works.
Follow-up of https://github.com/ros2/rosidl_python/pull/60.
Example: Run in one terminal:
in another terminal:
Output:
Expected output: