Add DSL-like methods

[dpc]

This is an initial implementation of the idea described in #80

The goal is to make all these generated types much more usable directly in code in a semi-DSL way with decent ergonomics.

Since there were concerns about compilation time increase, everything here is behind a new dsl cargo feature flag. In my initial tests, enabling it increases the compilation time by around 10%.

You can read more about how I'm planning to use it and prototyping it: https://github.com/rustshop/rustshop/discussions/12

The exact details of naming and APIs are yet to be determined. I welcome any feedback and ideas.

Some TODOs for me to remember addressing:

• [ ] add documentation from a field in any method interacting with it as well, so that IDEs can display hover-over documentation.

[Arnavion]

I would still like to know what these functions give you that just regular struct update syntax does not.

In the issue you had written:

It doesn't compose well.

let pod_spec = PodSpec::default()
 .set_restart_policy("Always")
 .append_container(get_default_node_js_container_spec())

vs ...

let mut pod_spec = PodSpec::default();
pod_spec.restart_policy = "Always".to_string();
pod_spec.container.push(get_default_node_js_container_spec());

?

The equivalent way is not what you wrote, but:

let pod_spec = PodSpec {
    restart_policy: "Always".to_string(),
    containers: vec![get_default_node_js_container_spec()],
    ..Default::default()
};

Similarly, your rustshop discussion has:

ctx.add_service(app_name, |s| {
   s.metadata_with(|m| {
       m.labels()
           .insert("some_extra_service_label".into(), "iamaservice".into());
   })
   .spec()
   .ports_with(|ports| {
       ports.push(ServicePort {
           port: i32::from(common_app::DEFAULT_LISTEN_PORT),
           ..Default::default()
       })
   })
   .selector_set(pod_selector.clone());
})
.add_deployment(app_name, |d| {
   d.metadata_with(|m| {
       m.labels_insert_from(&labels);
       m.labels()
           .insert("some_extra_label".into(), "some_extra_value".into());
   })
   .spec()
   .replicas_set(1)
   .selector_set(pod_selector.clone())
   .template_with(|t| {
       t.metadata().labels_insert_from(&labels);
       t.metadata().name_set(app_name.to_owned());
       t.spec().containers_push_with(|c| {
           c.image_set("redis".to_owned()).name_set("main");
       });
   });
});

which could be:

ctx.add_service(app_name, Service {
    metadata: ObjectMeta {
        labels: Some([("some_extra_service_label".into(), "iamaservice".into())].into()),
        spec: Some(ServiceSpec {
            ports: Some(vec![
                ServicePort {
                    port: i32::from(common_app::DEFAULT_LISTEN_PORT),
                    ..Default::default()
                }
            ]),
            selector: Some(pod_selector.clone()),
            ..Default::default()
        }),
        ..Default::default()
    },
    ..Default::default()
})
.add_deployment(Deployment {
    metadata: ObjectMeta {
        labels: Some([("some_extra_label".into(), "some_extra_value".into())].into()),
        spec: Some(DeploymentSpec {
            replicas: Some(1),
            selector: Some(pod_selector.clone()),
            template: PodTemplateSpec {
                metadata: Some(ObjectMeta {
                    labels: Some(labels.clone()),
                    name: Some(app_name.clone()),
                    ..Default::default()
                }),
                spec: Some(PodSpec {
                    containers: vec![
                        Container {
                            image: Some("redis".to_owned()),
                            name: "main".to_owned(),
                            ..Default::default()
                        }
                    ],
                    ..Default::default()
                }),
                ..Default::default()
            },
            ..Default::default()
        }),
        ..Default::default()
    },
    ..Default::default()
})

In particular, you've added builders for what seems to me an arbitrary set of operations:

• Vec fields get an append_from(&mut self, impl Borrow<[T]>) -> &mut Self and Map fields get an insert_from(&mut self, impl Borrow<Map<String, T>>) -> &mut Self. Why that instead of .extend(&mut self, impl IntoIterator<T>) -> &mut Self and .extend(&mut self, impl IntoIterator<(K, V)>) -> &mut Self ? It is very un-Rust-y to take borrows when they will be converted to owned types. It means there will be a needless copy even if the caller had an owned value they were willing to give up.

  And if one were to change them to .extend, why could I not use the existing Vec::extend and BTreeMap::extend instead of this builder?

• Why do the push_with and insert_with need to exist? They just call the closure on a Default value; why couldn't they be fn push(&mut self, value: T) -> &mut Self and fn insert(&mut self, name: String, value: T) -> &mut Self ? And in that case why could I not use the existing methods on Vec and BTreeMap ?

• What's the reason to have these operations and not others? Why only adders but not removers or modifiers?

• It is already not as simple as what you had hoped for. PodSpec::restart_policy_set takes impl Into<Option<String>> which &str is not, so you can't do .restart_policy_set("Always"). You have to do .restart_policy_set("Always".to_string())

Again, I know that fluent APIs exist, but I don't see what place they have in a language that has struct update syntax + a library where all fields are public.

[dpc]

which could be:

Note that your example always overwrites everything.

In my example:

ctx.add_service(app_name, |s| {

a can be arbitrarily pre-populated. Eg. the higher level library might have already added common-labels, readiness/liveliness probes etc.

Since the time of publishing I've abstracted away some general templates which then the specific code customizes

And there's also:

                            ..Default::default()
                        }
                    ],
                    ..Default::default()
                }),
                ..Default::default()
            },
            ..Default::default()
        }),
        ..Default::default()
    },

:roll_eyes: :shrug:

It is very un-Rust-y to take borrows when they will be converted to owned types. It means there will be a needless copy even if the caller had an owned value they were willing to give up.

You might be right. I'm open for refining all the APIs here. Right now I just prototyped something that I can build with, and get some feedback. Not sure how much I want to invest into it if you'd be vehemently opposed to include it, even under feature flag, and I need to validate that it makes sense myself. :)

And if one were to change them to .extend, why could I not use the existing Vec::extend and BTreeMap::extend instead of this builder?

Notably the method I'm introducing take care of add adding Some automatically. The sheer number of Optional fields is a total PITA when working with these types. :sweat_smile:

It is already not as simple as what you had hoped for.

I agree that it probably won't be perfect, but I'll take 90/10 whenever I can.

[Arnavion]

a can be arbitrarily pre-populated. Eg. the higher level library might have already added common-labels, readiness/liveliness probes etc.

Okay, that makes sense.

It is very un-Rust-y to take borrows when they will be converted to owned types. It means there will be a needless copy even if the caller had an owned value they were willing to give up.

You might be right.

https://rust-lang.github.io/api-guidelines/flexibility.html#caller-decides-where-to-copy-and-place-data-c-caller-control

Notably the method I'm introducing take care of add adding Some automatically. The sheer number of Optional fields is a total PITA when working with these types. sweat_smile

I don't think saving the user Some() is enough of a reason, and those Options are anyway still there when you need to get data out of the objects. I'd rather not abstract such fundamentals away.

---

I think it makes sense to add a "deep-merge" API that overrides None's with Some's and concatenates Vecs and Maps. I'm not convinced about the fluent API itself, unless you have any other reasons.

[dpc]

I think it makes sense to add a "deep-merge" API that overrides None's with Some's and concatenates Vecs and Maps. I'm not convinced about the fluent API itself, unless you have any other reasons.

Oh... actually, maybe that would work for me as well or even better!

It also has a nice property, that one could deserialize a bunch yaml to a given type, and then merge it back, allowing not only handy in-Rust-code updates, but possibly building all sorts of yaml based things (e.g. something read from a file, or opened in $EDITOR for user to modify) .

So for every type, we would define fn merge_from(&mut Self, other: Self) that would run merge_from recursively on all fields?

Does this seem OK, or do you have any other details in mind?

[dpc]

If you are more inclined to land such an improvement, please let me know if I should try to build it with the core code that I already wrote (I had to add some extra type-tracking to deal with optionals etc.) or possibly if you want to try to do yourself in some ways that more suits you (long term maintenance is the more important aspect here).

[Arnavion]

So for every type, we would define fn merge_from(&mut Self, other: Self) that would run merge_from recursively on all fields?

Does this seem OK, or do you have any other details in mind?

Yes, exactly.

Also it should be fine to not have a feature flag for the methods.

@teozkr @lfrancke Since you commented on the original issue, what are your thoughts?

If you are more inclined to land such an improvement, please let me know if I should try to build it with the core code that I already wrote (I had to add some extra type-tracking to deal with optionals etc.) or possibly if you want to try to do yourself in some ways that more suits you (long term maintenance is the more important aspect here).

You can start first. If I want to tweak it I can do that myself or discuss with you before merging.

[lfrancke]

Thank you for pinging me. I will let Teo comment on this. He's much more involved in this now.

[dpc]

@Arnavion @lfrancke @teozkr A WIP (but compiling) version of "deep merge" pushed in #121 .

[Arnavion]

Superseded by #121