Skip to content
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

Merged
merged 1 commit into from Mar 16, 2022

Conversation

TedDriggs
Copy link
Collaborator

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.

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
@ijackson
Copy link
Contributor

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
Copy link
Collaborator Author

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.

@TedDriggs TedDriggs merged commit abc20ba into master Mar 16, 2022
@TedDriggs TedDriggs deleted the custom-struct-attrs branch March 16, 2022 22:03
@ijackson
Copy link
Contributor

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
Copy link
Collaborator Author

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.

ijackson added a commit to ijackson/rust-derive-builder that referenced this pull request Mar 17, 2022
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)
TedDriggs pushed a commit that referenced this pull request Mar 21, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support arbitrary struct and impl-block attributes
2 participants