Declare fn new in waitables as pub(crate)

[nnmm]

This PR restricts the visibility of fn new in waitables (Subscription, Client, Service) so that only other code in the crate can access it. This prevents users from instantiating these structures and therefore we ensure that the only way of creating them is via the Node::create_X methods so that any internal logic is not bypassed.

[esteve]

I prefer to keep the code that adds the entity in this method, not in ::new, that wawy we don't need to make Node mutable there.

[nnmm]

It doesn't matter where the code is, no? As long as both functions end up adding the entity to the vec in the node, the node will need to be mutable.

[esteve]

It does matter. The Client shouldn't need to modify a member of the Node, unless it absolutely needs, it breaks the encapsulation. I find it easier to maintain and to read if only the struct that holds the members is the one that modifies them.

[nnmm]

OK, let me clarify: It doesn't matter for the question of whether the Node argument is passed by &mut.

[esteve]

In any case, it's still cleaner to modify any of the Node members inside the Node struct than letting other structs change them. However, I'd like to understand your rationale for modifying this code, since it doesn't seem to me that it provides any advantage, and makes it less readable and maintainable. Could you elaborate why you've moved it to the ::new method? Thanks.

[esteve]

If the rationale for moving this code to Client::new is to ensure that even calling when calling the Client constructor directly it gets added to the Node's client vector, please add that to the description. PRs should explain what it is they try to improve/fix, how it's done and why it's done.

Edit: if this is the rationale for moving the code, I'm not convinced. The expected behavior of the ROS API (see https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/create_client.hpp#L69-L82) is to always go through Node::create_X, not create the the entities directly, so I prefer to stick to what users would expect from other client libraries.

[nnmm]

That code two comments up is what we did before this PR, and yes, that is exactly the rationale for the change. I added a sentence to the PR description.

If we only want to provide create_x – I'm also fine with making X::new() have pub(crate) visibility.

[esteve]

In that case, I prefer to keep the current code in create_X and declare X::new() have pub(crate) visibility. Please, for future PRs, add more context, it wasn't straightforward to understand what you wanted to address. I've found that PRs lately are less descriptive, which makes reviews longer until we find out what's the rationale.

[nnmm]

I was a bit lazy on the description because we discussed this issue in chat, but you're right, I should add more detail anyway.

[esteve]

No problem about the description, it's just it was hard to track the rationale from the chat. I've reread the chat and see why you submitted this PR, thanks!