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

Fix import statement and usage for rclpy.node.Node #196

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

mikaelarguedas
Copy link
Member

node is not imported in __init__.py anymore (ros2/rclpy#147)

Without this change the demos raise:
AttributeError: module 'rclpy' has not attribute 'Node'

Not sure if ros2/rclpy#147 is a long term fix as this mean that anyone inheriting from Node will need to import it explicitly.

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Nov 27, 2017
@mikaelarguedas mikaelarguedas self-assigned this Nov 27, 2017
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 27, 2017
Copy link
Member

@dhood dhood left a comment

Choose a reason for hiding this comment

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

The name of the PR suggests to me that we are changing the demos to inherit from node in this PR; for clarity when searching PRs in the future could you simplify it?

@mikaelarguedas
Copy link
Member Author

The name of the PR suggests to me that we are changing the demos to inherit from node in this PR; for clarity when searching PRs in the future could you simplify it?

Sure, do you have a suggestion?

@wjwwood
Copy link
Member

wjwwood commented Nov 27, 2017

Sure, do you have a suggestion?

"Fix import statement and usage for rclpy.node.Node"?


This is really unfortunate because it adds another line to all of the examples unnecessarily (imo). It is also asymmetric with what we want to do with the C++ API, i.e. we want rclcpp::Node rather than rclcpp::node::Node. I don't understand fully why @dirk-thomas needed to do what he did in ros2/rclpy#147 (comment), but I think it's a step backwards for the usability of the rclpy API.

@mikaelarguedas
Copy link
Member Author

"Fix import statement and usage for rclpy.node.Node"?

Sounds good to me


This is really unfortunate because it adds another line to all of the examples unnecessarily (imo). It is also asymmetric with what we want to do with the C++ API, i.e. we want rclcpp::Node rather than rclcpp::node::Node. I don't understand fully why @dirk-thomas needed to do what he did in ros2/rclpy#147 (comment), but I think it's a step backwards for the usability of the rclpy API.

Yeah I agree, I'm proposing this fix mostly to get the demos back to a working state but don't think users should have to import this explicitly in the long-term.
I'd like to merge this before next week but can open a follow-up issue to revisit this before the release. How does that sound ?

@mikaelarguedas mikaelarguedas changed the title Import rclpy.node.Node and inherit from Node Fix import statement and usage for rclpy.node.Node Nov 27, 2017
@wjwwood
Copy link
Member

wjwwood commented Nov 27, 2017

+1 for getting things working again, but we should try to address this before the release I think.

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.

3 participants