tychedelia/kafka-protocol-rs

Fallible message builder lacks ergonomics

thedodd opened this issue · 4 comments

As far as I can tell, there is no reason for the builders to be fallible. The framework being used has this pattern in place to support things like validation and other patterns; however, none of that appears to be applicable here.

I've found myself using let mut res = ResponseMsg::default(); and then manually building the response object this way, because it is not fallible, and can be done in fewer lines of code.

IMHO, it would be a win to replace the builder framework with something non-fallible which starts with a default instance.

Thoughts?

I agree this is annoying but is unfortunately an outstanding issue in the upstream crate. See colin-kiegel/rust-derive-builder#56. I'll leave this open as it's worth fixing if this ever gets changed in the upstream.

bkirwi commented

Have folks considered code-generating the builders instead of relying on a third-party crate? Aside from avoiding the troublesome dep, may have some compile-time benefit...

Have folks considered code-generating the builders instead of relying on a third-party crate? Aside from avoiding the troublesome dep, may have some compile-time benefit...

Would definitely accept a contribution here! Given our existing codegen footprint, I agree that this would be preferable rather than relying on a third party crate (that makes assumptions/decisions we disagree with). Personally, I prefer working with builders and would like to see them as the first-line API for working with this crate, while still supporting the mutable Default::default pattern for users who need it.

We've replaced this with our own with_* methods that can be chained off Default. Further improvements to our in-house builder can be filed as new features!