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.

Sign up for GitHub

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

Jump to bottom

Solved bug when trying to find node info in namespaces #206

Merged
merged 5 commits into from
Mar 15, 2019
Merged

Solved bug when trying to find node info in namespaces #206

merged 5 commits into from
Mar 15, 2019

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Mar 13, 2019 •
edited
Loading

Fixes #201
Fixes #181
Initial '/' was missing when the node was inside a namespace.
Now, non fully qualified names are also accepted.
Added unit test.

@ivanpauno
Solved bug when trying to find node info in namespaces
678c158 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Mar 13, 2019
@ivanpauno ivanpauno changed the title Solved bug when trying to find node info in namespaces (#201) Solved bug when trying to find node info in namespaces Mar 13, 2019
@ivanpauno
Added test
e1215c7 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Maybe, I should also correct #181 here

dirk-thomas
dirk-thomas reviewed Mar 13, 2019
View reviewed changes
ros2node/ros2node/api/__init__.py Outdated
namespace = '/'.join(tokens[:-1])
return NodeName(node_name, namespace, full_node_name)
namespace = '/' + '/'.join(tokens[:-1])
return NodeName(node_name, namespace, '/' + full_node_name)
Copy link
Member

Choose a reason for hiding this comment

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

The terminology seems to be all mixed in this function. The parameter full_node_name must explicitly not have a leading slash. But the returned value of NodeName.full_name explicitly does.

While not part of this patch the condition 1 > len(tokens) can never be true. Also some callers explicitly remove the leading slash before calling the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's right. Maybe I should do the following:

  • Change callers to not remove the leading slash.
  • Also fix [ros2node] info requires fully qualified node name #181 here, adding a leading slash to non fully qualified names (inside parse_node_name).
  • Change "parse_node_name(full_node_name)" to "parse_node_name(node_name)" and node_name to "bare_node_name" or something similar.

Copy link
Member

Choose a reason for hiding this comment

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

👍

... and node_name to "bare_node_name" or something similar.

Maybe node_basename to match terminology for paths?

@ivanpauno
Accepted non fully qualified names. Used better terminology for varia…
33d3fd6 
…bles.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

@dirk-thomas The failing test is unrelated, so I will merge if you think it is ok.

dirk-thomas
dirk-thomas reviewed Mar 14, 2019
View reviewed changes
ros2node/ros2node/api/__init__.py Outdated
node_name = full_node_name
namespace = '/'
def parse_node_name(node_name):
if (node_name[0] == '/'):
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: no need for parenthesis around conditions.

ros2node/ros2node/api/__init__.py Outdated Show resolved Hide resolved
@ivanpauno
Modified with PR comments
3380af0 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

New CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dirk-thomas
Copy link
Member

I simplified the logic a bit in c5c72d4. That unfortunately broke the DCO check. Feel free to override it (or squash before merging to make it pass again).

@dirk-thomas @ivanpauno
simplify logic
4efa4ed 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

Last CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

Merging, as CI failures are unrelated

@ivanpauno ivanpauno merged commit 9ea0672 into ros2:master Mar 15, 2019
@ivanpauno ivanpauno removed the in progress Actively being worked on (Kanban column) label Mar 15, 2019
@Karsten1987
Copy link
Contributor

Could it be that this issue still persists?

 ➭ ros2 node info /twist_publisher
/twist_publisher
  Subscribers:
    /parameter_events: rcl_interfaces/ParameterEvent
  Publishers:
    /cmd_vel: geometry_msgs/Twist
    /parameter_events: rcl_interfaces/ParameterEvent
    /rosout: rcl_interfaces/Log
  Services:
    /twist_publisher/describe_parameters: rcl_interfaces/DescribeParameters
    /twist_publisher/get_parameter_types: rcl_interfaces/GetParameterTypes
    /twist_publisher/get_parameters: rcl_interfaces/GetParameters
    /twist_publisher/list_parameters: rcl_interfaces/ListParameters
    /twist_publisher/set_parameters: rcl_interfaces/SetParameters
    /twist_publisher/set_parameters_atomically: rcl_interfaces/SetParametersAtomically

when invoked without / I'll get:

 ➭ ros2 node info twist_publisher
Unable to find node 'twist_publisher'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ros2node] info fails to find nodes in namespaces [ros2node] info requires fully qualified node name
4 participants
@ivanpauno @dirk-thomas @Karsten1987 @tfoote