WIP: Replace builder to improve ergonomics

[Hackzzila]

This is my take on #41. It replaces the builders with methods like this:

    pub fn with_client_software_name(mut self, value: StrBytes) -> Self {
        self.client_software_name = value;
        self
    }

Then you can use Default and still have builder-like code.

Examples

Before:

    let request = ProduceRequest::builder()
        .acks(1)
        .timeout_ms(5000)
        .topic_data(
            [(
                topic_name.clone(),
                TopicProduceData::builder()
                    .partition_data(vec![PartitionProduceData::builder()
                        .index(0)
                        .records(Some(records))
                        .build()
                        .unwrap()])
                    .build()
                    .unwrap(),
            )]
            .into(),
        )
        .build()
        .unwrap();

After:

    let request = ProduceRequest::default()
        .with_acks(1)
        .with_timeout_ms(5000)
        .insert_topic_data(
            topic_name.clone(),
            TopicProduceData::default().append_partition_data(
                PartitionProduceData::default()
                    .with_index(0)
                    .with_records(Some(records)),
            ),
        );

---

Before:

    let request = FetchRequest::builder()
        .topics(vec![FetchTopic::builder()
            .topic(topic_name.clone())
            .partitions(vec![FetchPartition::builder()
                .partition(0)
                .fetch_offset(0)
                .build()
                .unwrap()])
            .build()
            .unwrap()])
        .build()
        .unwrap();

After:

    let request = FetchRequest::default().append_topics(
        FetchTopic::default()
            .with_topic(topic_name.clone())
            .append_partitions(
                FetchPartition::default()
                    .with_partition(0)
                    .with_fetch_offset(0),
            ),
    );

Open Questions

• Is this even wanted?
• What doc comments should be put on these methods?
• How to handle lists/maps?

RIght now the following methods are generated for Vec<T> types (and similar for Maps, but with a key)

pub fn with_topics(mut self, value: Vec<FetchTopic>) -> Self
pub fn append_topics(mut self, value: FetchTopic) -> Self
pub fn reserve_topics(mut self, additional: usize) -> Self

Alternatively (or in addition) you could add a extend_{field} method that takes impl IntoIterator<Item = {type}>

[Hackzzila]

Before I proceed with this, I'll wait for opinions from @tychedelia and @rukai

[rukai]

Personally I think this is fantastic and should give a huge improvement to ergonomics and hopefully a nice improvement to compile times too.

I'll take a closer look at the list/maps open question when I get some time.

[tychedelia]

Definitely in favor of removing the proc macro and rolling our own. I much prefer the builder versions personally, so I think it's worth thinking about what the right API is.

The main thing I'd like us to get right is accepting Into conversions where it makes sense as well as a good pattern for collections. My sense is that insert_foo style methods are less helpful when you're trying to map another collection into a message field, although they could definitely still be useful for more imperative message building.

One thing I might suggest is just running cargo expand and seeing any interesting tricks derive_builder is using that we could copy.

Would appreciate others thoughts and will try to think about this more closely this week. :)

[rukai]

I think the insert_*/append_*/reserve_* methods are a misstep.

Most likely you have one of:

• with_partition_data(partitions.iter().map(..).collect())
• with_partition_data(vec!(..))

Both of which will just preallocate the resulting vec to the exact required size.

Worst case you need to do something like:

  let partitions = vec!();
  if some_condition {
    partitions.insert(..)
  }
  TopicProduceData::default().with_partition_data(partitions);

Which I think is a reasonable trade off for keeping our builder API simple.

[rukai]

What doc comments should be put on these methods?

Doc comments would be great.
Something like:

/// Sets the $field to the passed value.
/// $documentation_for_field_taken_from_the_json

[Hackzzila]

I initially wanted insert_* and such mainly for indexmap, but I didn't realize they have a macro! I have removed that all now. I looked through the expanded code and didn't see anything that stood out to me.

I believe the last thing is figuring out if these methods should take a generic Into<T> parameter or not. If so, how do you handle options? Into<Option<T>> can get annoying when you just want to pass None, so I would vote for Option<Into<T>> (not the right syntax but you get my point).

[rukai]

I had a go at porting our project to the new builder API: it compiles, all tests pass and the code is much cleaner.
shotover/shotover-proxy#1701

Personally I would vote to not make use of Into in the API, I much rather a simple API and then the user can call .into() as needed.
But I am fine with being outvoted.

[rukai]

How do we feel about landing this PR as it is and then exploring further changes in follow up PRs?
To me it feels like a very solid base that is already an improvement above the status quo.

[tychedelia]

Hey, sorry, for some reason I got unsubscribed from the PR and was on vacation. Let me give this a look today and will merge if it seems like a good starting point. I'd like to release the next milestone for y'all!

[rukai]

The generator implementation looks good to me.
I tested the readme sample code compiles as well.

[tychedelia]

There are some minor nits I have still re: the redundancy of with_ prefix, but this looks fine to me and changing most of this in the future would be a mechanical refactor in a breaking release so probably fine. Agree this is a good base for future improvements, so am going to merge as is! Thanks for taking this on @Hackzzila

[tychedelia]

@rukai @Hackzzila released 0.11.0 https://crates.io/crates/kafka-protocol/0.11.0!

[rukai]

thanks!