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

qt_dotgraph: Small fixes for Python3 compatibility #88

Merged
merged 2 commits into from
Jul 21, 2017
Merged

qt_dotgraph: Small fixes for Python3 compatibility #88

merged 2 commits into from
Jul 21, 2017

Conversation

kartikmohta
Copy link
Contributor

@kartikmohta kartikmohta commented Jun 2, 2017

Mainly to get rqt_graph working with Python3. ros-visualization/rqt_graph#10 is also required for rqt_graph to work with Python3.

Copy link
Contributor

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

With the dotcode being expected to be bytes do these two strings also need to be prefixed with a b?

and

@@ -135,7 +135,9 @@ def getNodeItemForNode(self, node, highlight_level):
if name is None:
print("Error, no label defined for node with attr: %s" % node.attr)
return None
name = name.decode('string_escape')

import codecs # To work with Python 2 and 3 (https://stackoverflow.com/a/23151714)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move the import to the top of the file (between line 32 and 33) and remove the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kartikmohta Can you please make this small adjustment in order to get this patch merged.

@@ -53,7 +53,7 @@ def check_x_server():

class DotToQtGeneratorTest(unittest.TestCase):

DOT_CODE = '''
DOT_CODE = r'''
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why this change is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a '\N' in the string and that causes issues with Python3 since it tries to parse that as a unicode control character. To avoid that, it needs to be specified to be a raw string.

$ python3
>>> print('\N')
  File "<stdin>", line 1
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-1: malformed \N character escape
>>> print(r'\N')
\N

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying.

@dirk-thomas
Copy link
Contributor

Thank you for improving Python 3 support!

@dirk-thomas dirk-thomas merged commit 3084a93 into ros-visualization:kinetic-devel Jul 21, 2017
@kartikmohta kartikmohta deleted the fix/qt_dotgraph-python3 branch July 21, 2017 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants