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

Remove e.g. node:: namespaces and namespace escalation #416

Merged
merged 19 commits into from
Dec 5, 2017

Conversation

dhood
Copy link
Member

@dhood dhood commented Dec 4, 2017

This is following a decision we made Nov '16 but hadn't followed up on. Summary in #410 (comment).

The reasoning for doing it now is because doing it while in beta we won't have to deprecate the old format. If that's too aggressive we can look into the suggestion in #410 (comment).

My opinion on the priority is that this should be in before we tag but doesn't necessarily need to be in before testing since it will be clear from compilation whether or not it's been done correctly. I have a bunch of changes to other packages ready.

So far I've removed namespaces that only had classes. They are not always exactly matching the class name e.g. parameter_client:: had AsyncParamtersClient and SyncParametersClient, so let me know if I went too extreme and we can easily revert individual commits.

  • publisher::
  • subscription::
  • client::
  • service::
  • parameter_client::
  • paramter_service::
  • rate::
  • timer::
  • node::
  • any_service_callback::
  • any_subscription_callback::
  • context::
  • event::
  • executors::single_threaded_executor::SingleThreadedExecutor -> executors::SingleThreadedExecutor
  • executors::multi_threaded_executor::MultiThreadedExecutor -> executors::MultiThreadedExecutor

I removed the now-unnecessary namespace escalations from rclcpp/rclcpp.hpp but left the includes for those files because FWIU we still want users to not have to include the headers themselves.


There are some namespaces that have more than just classes so I wanted to check what to do with them:

  • executor:: has executor::Executor the class, but also:

    • executor::AnyExecutable a struct
    • executor::FutureReturnCode an enum
    • executor::ExecutorArgs a struct
  • parameter:: has

    • parameter::ParameterVariant class
    • parameter::ParameterType an enum
    • parameter::operator<<

Perhaps just move rclcpp::executor::Executor -> rclcpp:Executor and leave the others as is?

@dhood dhood added in progress Actively being worked on (Kanban column) question Further information is requested labels Dec 4, 2017
@dhood dhood self-assigned this Dec 4, 2017
@dhood dhood force-pushed the remove_ns_escalation branch 2 times, most recently from 7fde6ac to 30f930e Compare December 4, 2017 22:02
@dhood dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Dec 5, 2017
Copy link
Member

@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.

Perhaps just move rclcpp::executor::Executor -> rclcpp:Executor and leave the others as is?

I don't have an opinion on this. It sounds fine to me to only move the Executor up.

But maybe @wjwwood wants to jump in on this since he is more familiar with the rclcpp API.

@wjwwood
Copy link
Member

wjwwood commented Dec 5, 2017

Doesn't matter to me either, I don't expect normal users to use those other classes. Long term we should probably just put them each in their own files, at which point we might want to put them in a subfolder like executor or something, but for now I don't think it matters.

Even Executor is sort of moot because users will always use the single threaded or multi threaded executor, unless they're making their own which inherits from Executor.

@dhood
Copy link
Member Author

dhood commented Dec 5, 2017

ok I'll leave the executor and parameter namespaces as is. Users do interact with parameter::ParameterVariant but we can address it later if appropriate.

@wjwwood I forgot to ask: what do we want done with rclcpp::ok() and friends? https://github.com/ros2/rclcpp/pull/416/files#diff-690567624ee368716009118718985564R157
Remove the utilities namespace, or stick to the escalation?

@wjwwood
Copy link
Member

wjwwood commented Dec 5, 2017

Remove them I think, they're just in a utilities file, not a utilities folder right?

@dhood
Copy link
Member Author

dhood commented Dec 5, 2017

correct (but most other things we've spoken about were classes not free functions). I'll move em up!

@dhood dhood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Dec 5, 2017
@dhood dhood removed the question Further information is requested label Dec 5, 2017
@dhood
Copy link
Member Author

dhood commented Dec 5, 2017

Good to review now

CI for this change and all connected PRs that change usage to not include removed namespaces:

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

Turtlebot ci including many more repos:
Build Status

@wjwwood (and any others) I can help with adapting rviz to work with this change or hold off merging until appropriate

@dhood dhood merged commit 6129a12 into master Dec 5, 2017
@dhood dhood deleted the remove_ns_escalation branch December 5, 2017 23:02
@dhood dhood removed the in progress Actively being worked on (Kanban column) label Dec 5, 2017
dhood added a commit to ros2/examples that referenced this pull request Dec 5, 2017
dhood added a commit to ros2/geometry2 that referenced this pull request Dec 5, 2017
connects to ros2/rclcpp#416
* Remove ::publisher:: namespace

* Remove subscription:: namespace

* Remove service:: namespace

* Remove rate:: namespace

* Remove node:: namespace
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
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.

None yet

4 participants