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

Escalate more namespaces e.g. rclcpp::Service #410

Merged
merged 1 commit into from
Nov 30, 2017
Merged

Conversation

dhood
Copy link
Member

@dhood dhood commented Nov 30, 2017

Publishers and subscribers had been escalated but not services and clients. This was causing inconsistent usage in demos

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

@dhood dhood added the in review Waiting for review (Kanban column) label Nov 30, 2017
@dhood dhood self-assigned this Nov 30, 2017
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

using rclcpp::subscription::Subscription;
using rclcpp::subscription::SubscriptionBase;
using rclcpp::client::Client;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: looks like these were listed alphabetically originally

Copy link
Member Author

Choose a reason for hiding this comment

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

not really (e.g. subscription before rate), so I chose to continue "grouping" them

Copy link
Member

Choose a reason for hiding this comment

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

works for me 👍

@Karsten1987
Copy link
Contributor

Maybe only somewhat related, but I remember that we actually agreed on replicating the namespaces according the folder structure in which the classes are defined. That is to say, that instead of forwarding the namespaces, we should maybe in a follow-up PR remove the namespaces.

@dhood
Copy link
Member Author

dhood commented Nov 30, 2017

I dug up the conversation:

[wjwwood] So, +1 for a namespace per folder and not for a namespace per folder and file?

[Karsten1987] yes, that's correct.

[dirk-thomas] I agree[...] The namespace hierarchy should follow the folder hierarchy. Since we usually put one class into one file I don't think the file needs its own namespace. With that it should be straight forward to find the code of any type.

So it seems you are right @Karsten1987 that that was the conclusion, and that we should remove this namespaces.

I created this PR for the use case ros2/demos#199, so that demos wouldn't use the sub namespaces. That's something that we want to promote in our demos regardless, so I will merge this to support the demos, and then when we find time we should be safe to remove the namespaces from these classes without impacting the demos.
User code using the namespaced classes will break when we remove them so we should aim to do that by the release

@wjwwood
Copy link
Member

wjwwood commented Nov 30, 2017

+1 for merging this in order to fix the calling code and then get to removing the namespaces in rclcpp if we have time to do so.

@dhood dhood mentioned this pull request Nov 30, 2017
2 tasks
@dhood dhood merged commit 3b06aa3 into master Nov 30, 2017
@dhood dhood deleted the escalated_namespaces branch November 30, 2017 21:27
@dhood dhood removed the in review Waiting for review (Kanban column) label Nov 30, 2017
@wjwwood
Copy link
Member

wjwwood commented Dec 1, 2017

Btw, a slightly less aggressive thing to do for the longer namespaces in rclcpp (e.g. rclcpp::node::Node versus rclcpp::Node) is to invert the current case (change class to be rclcpp::Node and typedef to point to rclcpp::node::Node) and then deprecate the typedef's (which you can now do portably in C++14 😁), with the [[deprecated("message")]] attribute specifier syntax:

http://en.cppreference.com/w/cpp/language/attributes
http://josephmansfield.uk/articles/marking-deprecated-c++14.html

It's super neat.

@dhood
Copy link
Member Author

dhood commented Dec 1, 2017

that's a good idea because there will definitely be code that will break if we flatout remove the namespace

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
* Converted reinterpret_cast to static_cast where I felt it was safe-ish

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>

* Fixed inconsistent type inference

Signed-off-by: Jacob Hassold <jhassold@dcscorp.com>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
* disable play_filters_by_topic

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

* Add ticket to disabled test

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>

Co-authored-by: Tully Foote <tfoote@osrfoundation.org>

Co-authored-by: Tully Foote <tfoote@osrfoundation.org>
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.

4 participants