search

colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.28k stars 82 forks source link

Attribute pass-through for builder struct and impl block #241

Closed TedDriggs closed 2 years ago

TedDriggs commented 2 years ago

Following on from the addition of attribute pass-through for fields and setters, it's now possible to specify attributes for the struct using #[builder_struct_attr(...)] and for the inherent impl block using #[builder_impl_attr(...)].

With this change, container-level serde attributes are supported and #[wasm_bindgen] is possible, fixing #221 (though making the builder work with WASM remains the caller's responsibility).

Fixes #239

kudos to @ijackson for doing the hard work on this; @ijackson, if you can review this change that'd be much appreciated.

ijackson commented 2 years ago

Thanks for inviting my opinions :-). Looks good to me.

I have one comment: I found the Options::unnest_attrs function too similar to the existing function it was modeled on. I don't think that needs to block merging this, but: would you like me to have a go at refactoring to remove the duplication?

TedDriggs commented 2 years ago

I'd be interested, but I'd suggest starting with a GH issue that includes the signature of the proposed shared function.

I don't think trading some code duplication for a shared function that's hardcoded to having two buckets is a big improvement. For the generalized case, the best alternative I could come up with was this:

fn partition_and_unnest_attrs(unnest: &[&'static str], attrs: Vec<Attribute>) -> (HashMap<&'static str, Vec<Attribute>, Vec<Attribute>)

That didn't feel like enough of a win to be worth the added reading complexity.

ijackson commented 2 years ago

I'd be interested, but I'd suggest starting with a GH issue that includes the signature of the proposed shared function.

I was thinking something like:

fn partition_and_unnest_attrs(attrs: Vec<Attribute>, unnest: &[(&'static str, &mut Vec<Attribute>)]) -> Result
TedDriggs commented 2 years ago

First, passing observation: Neither of us was able to define this function's signature concisely enough to avoid it creating a horizontal scrollbar in the GH issues view. That's probably a bad sign :)

Would the output of that include cloned attributes destined for both locations? In that case it's not a true "partition". The whole thing feels very imperative at the moment, which makes naming a function for this harder and makes me wonder if the challenge of explaining this "function" is worth the 30 lines of duplicate code it removes.

I share your sense that this is poor hygiene/bad style, but I'm not sure the alternatives are better.