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

show connection info on rosnode info #1497

Conversation

maximest-pierre
Copy link
Contributor

Continuation of #1386
The only changes are to address the reviews

@maximest-pierre maximest-pierre changed the title show commit info on rosnode info show connection info on rosnode info Aug 28, 2018
@maximest-pierre maximest-pierre force-pushed the show_connection_info_in_rosnode_info branch from 4f76381 to 254e659 Compare August 28, 2018 02:39
@@ -62,6 +63,8 @@

NAME='rosnode'
ID = '/rosnode'
# Line is defined at clients/rospy/src/rospy/impl/tcpros_base.py at line 477
Copy link
Member

Choose a reason for hiding this comment

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

Please do not refer to a line number since that is going to change if the file is being edited in the future. Instead refer to the file and a symbol, e.g. The string is defined in clients/rospy/src/rospy/impl/tcpros_base.py TCPROSTransport.get_transport_info.

@@ -473,6 +473,7 @@ def get_transport_info(self):
Similar to getTransportInfo() in 'libros/transport/transport_tcp.cpp'
e.g. TCPROS connection on port 41374 to [127.0.0.1:40623 on socket 6]
"""
# Changing this line while break tools/rosnode/src/rosnode/__init__.py at line 67
Copy link
Member

Choose a reason for hiding this comment

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

Please do not refer to a line number since that is going to change if the file is being edited in the future. Instead refer to the file and a symbol, e.g. Pattern matching this output in tools/rosnode/src/rosnode/__init__.py CONNECTION_PATTERN.

Also this line currently uses a tab which can't be mixed with space in Python 3. Please change it to use the spaces (as the rest of the file).

@maximest-pierre maximest-pierre force-pushed the show_connection_info_in_rosnode_info branch from 254e659 to db2f425 Compare August 28, 2018 15:49
@maximest-pierre
Copy link
Contributor Author

@dirk-thomas It should be good now

@dirk-thomas
Copy link
Member

dirk-thomas commented Aug 28, 2018

Great, thank you for picking this up.

For future PRs it would be good to keep the original commit in the author name.

@dirk-thomas dirk-thomas merged commit 53d958f into ros:melodic-devel Aug 28, 2018
@efernandez
Copy link
Contributor

Thanks @maximest-pierre . Did you notice the rostopic info command being significantly slower after this change?

@maximest-pierre
Copy link
Contributor Author

@efernandez Not really. I just compared it with time both version and they were almost identical.

@efernandez
Copy link
Contributor

Then it should be fine @maximest-pierre . I probably have a laggy connection when I tried, or an overloaded system. 👍

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.

3 participants