Add escaping for strings which is valid in YAML #79
Add escaping for strings which is valid in YAML #79dirk-thomas merged 5 commits intoros:kinetic-develfrom
Conversation
package.xml
Outdated
| <run_depend>genmsg</run_depend> | ||
| <run_depend>python-yaml</run_depend> | ||
|
|
||
| <test_depend>python-yaml</test_depend> |
There was a problem hiding this comment.
With the added run dependency this test dependency need to be removed, see current CI failures:
The test dependency on "python-yaml" is redundant with: run_depend
|
Seems to build now ;-) |
src/genpy/message.py
Outdated
| return "''" | ||
| return val | ||
| # escape strings for use in yaml file using yaml dump | ||
| return yaml.dump(val).split('\n')[0] |
There was a problem hiding this comment.
Dorian, is there a particular reason to only print the first line? It would be important for me to also handle multi-line strings.
There was a problem hiding this comment.
This looks indeed like a problem. If the output of dump is being wrapped in the middle the first line on its own is even not valid.
There was a problem hiding this comment.
Sorry about the late reply, must have over looked the notification while traveling...
You're right, I did not fix it for multi-line strings, yet.
My last commit now solves this as well.
| return "''" | ||
| return val | ||
| # escape strings for use in yaml file using yaml dump with default style to avoid trailing "...\n" | ||
| return yaml.dump(val, default_style='"').rstrip('\n') |
There was a problem hiding this comment.
After reading the comment I have one question: with default_style='"' does it still need the rstrip('\n')?
There was a problem hiding this comment.
Yes, the new style just removes the "\n..." there is still an additional "\n" at the end...
test/test_genpy_message.py
Outdated
|
|
||
| def test_strify_yaml(self): | ||
| def roundtrip(m): | ||
| #print('\n\n\nroundtrip') |
There was a problem hiding this comment.
Can you please remove the added but commented lines.
|
We are aiming for a new round of releases early next week (Mon or Tue). It would be great f this would be ready before in order to get released. |
| # TODO: escape strings properly | ||
| elif isstring(val0): | ||
| # escape list of strings for use in yaml file using yaml dump | ||
| return yaml.dump(val).rstrip('\n') |
There was a problem hiding this comment.
Should this line also use default_style='"'?
There was a problem hiding this comment.
no, its not needed here since val is a list of strings which will never be serialized using the "\n...\n" format. (this format is used only for "simple" strings, with no characters that need escaping...)
|
Thank you for the patch! |
|
Thanks for the thorough reviews! |
Compatibility for ros/genpy#79.
Feeding output of "rostopic echo" back into "rostopic pub" does not work properly with strings containing characters which have special meaning in YAML (e.g. '#', "'" or '"').
These chars should be escaped properly to make the output of "rostopic echo" and the string representation of messages in general valid YAML code.
Strings are not properly escaped in strify_message, so publishing a string like e.g. 'foo # bar':
results in YAML code where the string only contains "foo " and "# bar" is treated as a comment, changing the data without even raising an error:
After applying this PR the correct result is printed
This also holds for enclosed "'" and '"' characters.
To test this I also updated the test_strify_yaml to check for proper escaping.