Make the output from fibonacci_client_py feedback and result look nicer

[ivanpauno]

Follow-up of ros2/rosidl_python#60.

Example:
Run in one terminal:

ros2 run action_tutorials fibonacci_action_server

in another terminal:

ros2 run action_tutorials fibonacci_action_client

Output:

[INFO] [fibonacci_action_client]: Received feedback: array('i', [0, 1, 1])

Expected output:

[INFO] [fibonacci_action_client]: Received feedback: [0, 1, 1]

[ivanpauno]

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.

[dirk-thomas]

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.

[ivanpauno]

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.

[dirk-thomas]

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.

[ivanpauno]

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.

[clalancette]

Ok. feedback.partial_sequence is of type array.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.

[dirk-thomas]

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 type array.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]

@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.

[ivanpauno]

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.

[clalancette]

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

demos/action_tutorials/action_tutorials_py/action_tutorials_py/fibonacci_action_client.py

Line 61 in 9765191

self.get_logger().info('Received feedback: {0}'.format(feedback.partial_sequence))

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?

[ivanpauno]

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.