Remove e.g. node:: namespaces and namespace escalation#416
Conversation
7fde6ac to
30f930e
Compare
30f930e to
0b8041a
Compare
dirk-thomas
left a comment
There was a problem hiding this comment.
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.
|
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 Even |
Users can use the directive themselves if they want
0b8041a to
8c9eb0f
Compare
|
ok I'll leave the @wjwwood I forgot to ask: what do we want done with |
|
Remove them I think, they're just in a utilities file, not a utilities folder right? |
|
correct (but most other things we've spoken about were classes not free functions). I'll move em up! |
bd7d9a8 to
d2019fb
Compare
d2019fb to
9d80e95
Compare
|
Good to review now CI for this change and all connected PRs that change usage to not include removed namespaces:
Turtlebot ci including many more repos: @wjwwood (and any others) I can help with adapting rviz to work with this change or hold off merging until appropriate |
connects to ros2/rclcpp#416
connects to ros2/rclcpp#416 * Remove ::publisher:: namespace * Remove subscription:: namespace * Remove service:: namespace * Remove rate:: namespace * Remove node:: namespace
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
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::hadAsyncParamtersClientandSyncParametersClient, 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::SingleThreadedExecutorexecutors::multi_threaded_executor::MultiThreadedExecutor->executors::MultiThreadedExecutorI removed the now-unnecessary namespace escalations from
rclcpp/rclcpp.hppbut 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::hasexecutor::Executorthe class, but also:executor::AnyExecutablea structexecutor::FutureReturnCodean enumexecutor::ExecutorArgsa structparameter::hasparameter::ParameterVariantclassparameter::ParameterTypean enumparameter::operator<<Perhaps just move
rclcpp::executor::Executor->rclcpp:Executorand leave the others as is?