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

Regression in msg_generator when escaping trailing double-quotes #51

Merged
merged 1 commit into from
Mar 29, 2016

Conversation

dhood
Copy link
Contributor

@dhood dhood commented Mar 29, 2016

Does it make sense to add tests for genpy to check each of the std_msgs?

@dhood
Copy link
Contributor Author

dhood commented Mar 29, 2016

although, even Empty for me is giving full_text='\n'. But something else might show up (in light of 4f7755b#commitcomment-16880685)

@@ -772,7 +772,7 @@ def msg_generator(msg_context, spec, search_path):

full_text = compute_full_text_escaped(msg_context, spec)
# escape trailing double-quote, unless already escaped, before wrapping in """
if full_text[-1] == '"' and not full_text[-2:] == r'\"':
if len(full_text) > 0 and full_text[-1] == '"' and not full_text[-2:] == r'\"':
Copy link
Member

Choose a reason for hiding this comment

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

I thought about something similar yesterday 😉 but only tried it with a string of length 1 which the list slicing if obviously fine with...

Anyway I think we should make it even more simple and defensive now:

if full_text.endswith('"') and not full_text.endswith(r'\"'):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I hadn't seen this before! will use it from now on, thanks

@dirk-thomas
Copy link
Member

Adding checks for every std_msgs seems to be extensive. Especially since genpy can't depend on std_msgs and therefore would need to duplicate all message definitions. More test cases would always be good but in this case I think it's not strictly necessary. Fixing this logic problem should be "enough".

@dhood
Copy link
Contributor Author

dhood commented Mar 29, 2016

ok - here's hoping :)

@dirk-thomas
Copy link
Member

Great. Thanks!

@dirk-thomas dirk-thomas merged commit b7409d9 into ros:indigo-devel Mar 29, 2016
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.

None yet

2 participants