-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attribute pass-through for builder struct and impl block #241
Conversation
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
Thanks for inviting my opinions :-). Looks good to me. I have one comment: I found the |
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. |
I was thinking something like: fn partition_and_unnest_attrs(attrs: Vec<Attribute>, unnest: &[(&'static str, &mut Vec<Attribute>)]) -> Result |
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". I share your sense that this is poor hygiene/bad style, but I'm not sure the alternatives are better. |
Replaces two copies of the same code, in Field::unnest_attrs and Options::unnest_attrs. Also, replaces the double specification of the output attributes list variables in each of these functions (once when calculating unnest, and once when copying the fields to every output). Discussion was here, et seq: colin-kiegel#241 (comment)
Replaces two copies of the same code, in Field::unnest_attrs and Options::unnest_attrs. Also, replaces the double specification of the output attributes list variables in each of these functions (once when calculating unnest, and once when copying the fields to every output). Discussion was here, et seq: #241 (comment) * Adjust comment * Add precondition assert As suggested in https://github.com/colin-kiegel/rust-derive-builder/pull/243/files/1db242c5a6b0c4066c58b48408e2ccd0da8ee8f9#diff-fce05abe2a1a48a344afbebd38b64914fde1d4131faa01fa9d32e5804b6f31ee
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.