From 25eabf745aad044675819445b2dad45610522dbd Mon Sep 17 00:00:00 2001
From: Nikolai Morin <nnmmgit@gmail.com>
Date: Thu, 21 Jul 2022 17:01:02 +0200
Subject: [PATCH] Declare fn new in waitables as pub(crate) (#227)

* Always add subscription/client/service to the node's list of waitables

* Revert changes. Declare waitables new function with pub(crate) visibility

* Document why ::new needs pub(crate) visibility

* Fix clippy issues

Co-authored-by: Esteve Fernandez <esteve@apache.org>
---
 rclrs/src/node/client.rs       |  7 ++++++-
 rclrs/src/node/service.rs      |  7 ++++++-
 rclrs/src/node/subscription.rs | 14 +++++++++-----
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/rclrs/src/node/client.rs b/rclrs/src/node/client.rs
index 17973052..b8c46d2e 100644
--- a/rclrs/src/node/client.rs
+++ b/rclrs/src/node/client.rs
@@ -56,6 +56,9 @@ type RequestValue<Response> = Box<dyn FnOnce(Response) + 'static + Send>;
 type RequestId = i64;
 
 /// Main class responsible for sending requests to a ROS service.
+///
+/// The only available way to instantiate clients is via [`Node::create_client`], this is to
+/// ensure that [`Node`]s can track all the clients that have been created.
 pub struct Client<T>
 where
     T: rosidl_runtime_rs::Service,
@@ -70,7 +73,9 @@ where
     T: rosidl_runtime_rs::Service,
 {
     /// Creates a new client.
-    pub fn new(node: &Node, topic: &str) -> Result<Self, RclrsError>
+    pub(crate) fn new(node: &Node, topic: &str) -> Result<Self, RclrsError>
+    // This uses pub(crate) visibility to avoid instantiating this struct outside
+    // [`Node::create_client`], see the struct's documentation for the rationale
     where
         T: rosidl_runtime_rs::Service,
     {
diff --git a/rclrs/src/node/service.rs b/rclrs/src/node/service.rs
index 968e3902..5603149d 100644
--- a/rclrs/src/node/service.rs
+++ b/rclrs/src/node/service.rs
@@ -55,6 +55,9 @@ type ServiceCallback<Request, Response> =
     Box<dyn Fn(&rmw_request_id_t, Request) -> Response + 'static + Send>;
 
 /// Main class responsible for responding to requests sent by ROS clients.
+///
+/// The only available way to instantiate services is via [`Node::create_service`], this is to
+/// ensure that [`Node`]s can track all the services that have been created.
 pub struct Service<T>
 where
     T: rosidl_runtime_rs::Service,
@@ -69,7 +72,9 @@ where
     T: rosidl_runtime_rs::Service,
 {
     /// Creates a new service.
-    pub fn new<F>(node: &Node, topic: &str, callback: F) -> Result<Self, RclrsError>
+    pub(crate) fn new<F>(node: &Node, topic: &str, callback: F) -> Result<Self, RclrsError>
+    // This uses pub(crate) visibility to avoid instantiating this struct outside
+    // [`Node::create_service`], see the struct's documentation for the rationale
     where
         T: rosidl_runtime_rs::Service,
         F: Fn(&rmw_request_id_t, T::Request) -> T::Response + 'static + Send,
diff --git a/rclrs/src/node/subscription.rs b/rclrs/src/node/subscription.rs
index 6a4af790..fa27302b 100644
--- a/rclrs/src/node/subscription.rs
+++ b/rclrs/src/node/subscription.rs
@@ -59,6 +59,9 @@ pub trait SubscriptionBase: Send + Sync {
 /// When a subscription is created, it may take some time to get "matched" with a corresponding
 /// publisher.
 ///
+/// The only available way to instantiate subscriptions is via [`Node::create_subscription`], this
+/// is to ensure that [`Node`]s can track all the subscriptions that have been created.
+///
 /// [1]: crate::spin_once
 /// [2]: crate::spin
 pub struct Subscription<T>
@@ -76,12 +79,14 @@ where
     T: Message,
 {
     /// Creates a new subscription.
-    pub fn new<F>(
+    pub(crate) fn new<F>(
         node: &Node,
         topic: &str,
         qos: QoSProfile,
         callback: F,
     ) -> Result<Self, RclrsError>
+    // This uses pub(crate) visibility to avoid instantiating this struct outside
+    // [`Node::create_subscription`], see the struct's documentation for the rationale
     where
         T: Message,
         F: FnMut(T) + 'static + Send,
@@ -213,15 +218,14 @@ where
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::{create_node, Context, Subscription, QOS_PROFILE_DEFAULT};
+    use crate::{create_node, Context, QOS_PROFILE_DEFAULT};
 
     #[test]
     fn test_instantiate_subscriber() -> Result<(), RclrsError> {
         let context =
             Context::new(vec![]).expect("Context instantiation is expected to be a success");
-        let node = create_node(&context, "test_new_subscriber")?;
-        let _subscriber = Subscription::<std_msgs::msg::String>::new(
-            &node,
+        let mut node = create_node(&context, "test_new_subscriber")?;
+        let _subscriber = node.create_subscription::<std_msgs::msg::String, _>(
             "test",
             QOS_PROFILE_DEFAULT,
             move |_: std_msgs::msg::String| {},