writing rosbags adds a newline#780
Conversation
|
Thank you for creating a regression test for this. The test itself looks good. I will comment on some minor style things inline. Regarding the proposed fix: as you mentioned it would probably be cleaner to fix |
test/test_rosbag/test/test_bag.py
Outdated
|
|
||
| inbag_filename = '/tmp/test_rosbag_filter__1.bag' | ||
| outbag1_filename = '/tmp/test_rosbag_filter__2.bag' | ||
| outbag2_filename = '/tmp/test_rosbag_filter__3.bag' |
There was a problem hiding this comment.
Please avoid hard coding temporary paths. Use tempfile instead. That will ensure that the names don't collide with other tests and potentially work on other platforms.
|
Thanks for the review @dirk-thomas - I'm understanding better now where the problem originated. I think the newline is trying to solve the same problem compute_full_text_escaped is. But, there was a bug in compute_full_text_escaped. What do you think about this instead? ros/genpy@indigo-devel...dhood:indigo-devel I haven't been able to find a message type that this will actually affect in order to test it properly though. Do you know of any? |
|
While the missing assignment certainly needs fixing a single quote at the end of the string also needs to be escaped. For the following input I don't know any message out of my head which has a trailing quote. Obviously none has triple quotes since escaping that correctly is broken. 😉 |
|
How does ros/genpy@indigo-devel...dhood:indigo-devel look now? I didn't bother with the beginning given the current msg format |
|
I'll open a PR on genpy so the tests can run |
|
Commits squashed. I think this can be closed now with ros/genpy#47 right? |
test/test_rosbag/test/test_bag.py
Outdated
| import sys | ||
| import time | ||
| import unittest | ||
| import tempfile |
There was a problem hiding this comment.
Please move this line two up (for alphabetical order of the core modules [PEP8]).
|
I think it still makes sense to merge this new test since it fill prevent regressions in this area in the future. |
|
thanks again - hopefully ok to merge now |
|
I will wait with the merge until |
|
@ros-pull-request-builder retest this please |
writing rosbags adds a newline
Potential fix for #736 where it was noticed that newlines are added each time rosbag filter is called.
From what I can see a newline is added to the message text here when writing a message, but it's never accounted for when reading the message. So it should either not be written, or it should be removed when reading.
Have removed the newline in the rosbag code but it may be appropriate to remove it here instead