Options pattern (spin-off of #427)

[mxgrey]

This PR is spun out of #427 and focuses entirely on introducing the "options" pattern for the various ROS primitives (subscription, publisher, service, and client).

This is a slight twist on the builder pattern that we were using via NodeBuilder up until now.

Instead of creating a "builder" object which will eventually .build() the final product, we will build an _Options structure which gets passed into a create_ function. Here are some examples:

• create_node
• create_subscription
• create_service

The user fills up the _Options with whichever fields are relevant to them, and the rest of the option fields will use default values. Note that publishers and subscribers will have different default QoS from clients and services, and this is taken into account by the option builders for these different primitive types.

[luca-della-vedova]

I personally like this API a lot more, makes it fairly easy to specify sensible defaults, as well as add new options in the future. Just two small comments

[esteve]

@mxgrey thanks for working on this. I have a question, why does create_node seem to be more explicit when accepting arguments (i.e. it requires a NodeOptions struct https://github.com/mxgrey/ros2_rust/blob/c46e2abc29da862f0a79dccad25a598852e44c8c/rclrs/src/test_helpers/graph_helpers.rs#L12-L13), whereas create_subscription and create_publisher do it more implicitly, by using IntoPrimitiveOptions (https://github.com/mxgrey/ros2_rust/blob/c46e2abc29da862f0a79dccad25a598852e44c8c/rclrs/src/node.rs#L384-L386).

Would it be possible to replace the latter two with a more explicit API? While I like how succinct it looks, at the same time I worry it's not as readable, it looks to me like the string to pass the name to the subscription/publisher has two additional methods ("my_service".keep_all().transient_local(), https://github.com/mxgrey/ros2_rust/blob/c46e2abc29da862f0a79dccad25a598852e44c8c/rclrs/src/node.rs#L384-L386)

[mxgrey]

why does create_node seem to be more explicit when accepting arguments

I implemented the NodeOptions before thinking of the way to implicitly cast a &str into an _Options instance. In #428 I committed 6059506 allows users to do executor.create_node("my_node") to match create_subscription and create_publisher, which unfortunately is the opposite direction of what you're asking for.

Would it be possible to replace the latter two with a more explicit API?

It's certainly possible, but making it explicit means users will always have to write out SubscriptionOptions::new("my_topic"), PublisherOptions::new("my_topic"), ServiceOptions::new("my_service"), etc, which I would see as additional noise with little benefit.

I think in most cases users will be copying off of example use cases, especially the examples we provide in the API docs of each method, so they wouldn't spend long being tripped up by the funny argument type. I'll admit that the "string".keep_all().transient_local() syntax is a little funny to look at, especially when it's all on one line. It looks a little more sensible when written like

let publisher = node.create_publisher(
    "my_topic"
    .keep_all()
    .transient_local(),
);

because then it's more clear that you're chaining "optional" arguments together.

I think we'll see a variety of opinions on whether the explicit vs implicit is preferable. We should definitely discuss it at the working group meeting tonight. I'd also suggest folks try writing some toy apps off of this branch to see how it feels.

[esteve]

@mxgrey could you fix the conflicts when you have a moment? From what I remember from the the WG meeting, we'll got with the Bevy way of using options ("str".option1().option2()). Could you update NodeOptions so that it behaves the same? Awesome work!

[mxgrey]

@esteve Let's start with reviewing and merging #428 before tackling this PR. Once #428 is merged, then I'll fix the merge conflicts for this PR. Otherwise I'll need to fix merge conflicts twice.

Could you update NodeOptions so that it behaves the same?

This is already taken care of in #428, so that will make its way into this PR after #428 is merged and I fix all the merge conflicts.

[maspe36]

Thanks for this great work Grey! Overall I really like this change, just a few comments, but so far LGTM!

[maspe36]

What about all of the other QoS policies associated with a QoSProfile? Comment and code don't seem to line up

[mxgrey]

Great catch, I had started writing this method before writing any of the others and meant to come back to it.

Fixed in this commit: d67f041

[maspe36]

would it be helpful to have a way to convert from PrimitiveOptions into QoSProfile?

i.e

impl From<PrimitiveOptions> for QoSProfile {
    fn from(options: PrimitiveOptions) -> Self {
        Self {
            history: options.history.unwrap_or(QOS_PROFILE_DEFAULT.history),
            ...
         }
     }
}

[mxgrey]

I would strongly discourage this for the following specific reason:

The idea of a "default" set of values for QoS is a bad one in my opinion. If it were entirely up to me, QOS_PROFILE_DEFAULT would not exist in rcl, and we would not provide QoSProfile::default(). The reason is, it should not be assumed that a single set of QoS settings would make the best default settings across both topics and services.

Currently QOS_PROFILE_DEFAULT and QOS_PROFILE_SERVICES_DEFAULT happen to have the same set of values, and QOS_PROFILE_DEFAULT is used as the default for topics, but by parading around QOS_PROFILE_DEFAULT as "the sensible default" across both topics and services, rcl has locked itself into hurting users if there's ever a shift in opinion about what the best default settings are for topics, and those settings aren't good for services, because there are probably many users out there who are using QOS_PROFILE_DEFAULT for services right now. Personally I think rcl should deprecate QOS_PROFILE_DEFAULT and replace it with QOS_PROFILE_TOPICS_DEFAULT.

That being said, maybe I'm being overly sensitive about this.

[maspe36]

Mmm fair enough. Lets take a step back here, what are we trying to accomplish?

We want to apply any QoS settings contained in the Option<> members of a PrimitiveOptions struct to either a QosProfile::topics_default() or a QoSProfile::services_default().

Currently, we expect the new() func for T in impl From<U> for T to have this information. Unfortunately though, because T owns the QoSProfile struct, we need to take a &mut to it and update it externally. Obviously, we aren't violating any safety guarantees, this will compile just fine. It was just unintuitive for me! Doesn't mean its that way for everyone.

Simplifying a bit, I personally would expect A.apply(B) to update or modify A, but in this API its the opposite.

[mxgrey]

Simplifying a bit, I personally would expect A.apply(B) to update or modify A, but in this API its the opposite.

Would it help if the API were expressed as PrimitiveOptions::apply_to(&mut QoSProfile)? I would have no problem with that at all.

I could also accept QoSProfile::apply(&mut PrimitiveOptions), but personally I'd rather the qos module not know anything about the primitive_options module since I'd rather have a simple clean dependency of primitive_options depends on qos rather than a circular dependency where they both depend on each other. Rust has no issue supporting circular dependencies between modules that are in the same crate, but it feels like bad hygiene to me.

[maspe36]

Yeah I think we can just rename apply to apply_to, that'll work

[mxgrey]

Updated here: a872cd6

[maspe36]

LGTM