Changes to `Activity` design

[vionya]

Some recent issues have brought up some concerns with the current structure of the Activity struct and the way it works. I'm currently working on re-designing these interfaces to address these issues and hopefully be overall easier to use. These changes are heavily breaking and would/will accompany a future major version update.

Changes

Note Activity and ActivityBuilder refer to all models provided by the library (i.e. Timestamps and TimestampsBuilder)

My current idea is to separate the current usage flow into distinct Activity and ActivityBuilder structs, implementing a consuming builder design pattern. My working design for this would look something like this in practice:

// A default `ActivityBuilder` supports chaining similar to that of the current `Activity` struct
let mut activity = activity::ActivityBuilder::default()
    .state("State")
    .details("Details")
    .assets(
        activity::AssetsBuilder::default()
            .large_image("large-image")
            .large_text("Large text")
            .build(),
    )
    .buttons(vec![activity::Button::new(
        "A button",
        "https://example.com",
    )])
    .timestamps(activity::TimestampsBuilder::default().start(1).build())
    .build();

// Individual fields on a built model can now be updated like so
activity.set_party(
    activity::PartyBuilder::default()
        .id("some-id")
        .size([1, 3])
        .build(),
);

This showcases several things:

• The ActivityBuilder structs support chaining similar to that of the current design
  • The setters are consumptive, so all calls to them leading up to .build() must be chained (or a new reference has to be created for each setter called)
• The Activity structs now have set_* methods which take an &mut self. This makes it much easier to update individual model values after the model has been built (#19)

Unrelated to the updates to chaining and construction, none of the library's models require lifetimes anymore, and accept anything that satisfies impl ToString where an &str was previously required - which was the reason the lifetimes were previously in place (also #19).

Discussion

I'm raising this issue to get feedback about the design as I've presented it, and if there are improvements to be had.

My main concern is the added boilerplate. I don't think it's a big issue for the large ActivityBuilder, since that's already a pretty long chain in practice, but as seen in the example code above, it definitely uglifies lines such as

    .timestamps(activity::TimestampsBuilder::default().start(1).build())

Is this beyond the acceptable amount of boilerplate, are there alternative solutions? One idea I scrapped was adding simple ::new functions for models like Timestamps instead of implementing a builder - the issue here is that this would then require arguments to be passed in an Option-friendly form, which still adds boilerplate.

---

This was a lot of words, so I thank any who read them all, and I'd appreciate feedback/discussion :)

[Bowarc]

Sorry for the very late response 😅

About the builder patern, check derive_builder if you're not using it yet

About boilerplate, would a impl of From<(Option<i64>, Option<i64>)> on Timestamps (for example) be usefull? That would create lines like

 .timestamps(activity::Timestamps::from((Some(1), None))

which imo is not that bad

[vionya]

Hey, appreciate any response at all 😄!

About the builder patern, check derive_builder if you're not using it yet

I've checked out that crate, and I really like what it does! My goal is to take design cues from the generated builders in the final product, absolutely. I'm slightly hesitant on including it as a dependency though, since it's not a difficult implementation, and I'd like to keep the crate relatively lean.

About boilerplate, would a impl of From<Option<i64>, Option<i64>) on Timestamps (for example) be usefull? That would create lines like

 .timestamps(activity::Timestamps::from((Some(1), None))

wich imo is not that bad

I really like this! It would make it a lot easier to construct these smaller models without tons of chained calls. Do you think this would be a good thing to implement alongside the verbose option (like how I constructed the Timestamps in the original issue)? That way, both options are available.

Thanks again for the response!

[Bowarc]

Late again 😅

Do you think this would be a good thing to implement alongside the verbose option (like how I constructed the Timestamps in the original issue)? That way, both options are available.

Yes! That would be preferable imo.

[vionya]

Alright, sounds good!

[Radiicall]

This is a good idea! I've been using this crate in my project for a while and I have been struggling with exactly this problem of ownership when trying to build an activity.

My current code for getting around this issue is very messy so having this fixed would be awesome!