Don't require FromMeta for field types, if the field is transformed
#204
Conversation
If a field is tranformed using `map` or `and_then`, there should not be a `FromMeta` bound on the actual field type, but only on the argument to the `map` or `and_then` function.
|
This changes the behaviour slightly since the fallback value is also transformed using either Hope this is not a problem. |
There was a problem hiding this comment.
This needs some better commenting and explanation, as well as some tests that explore the behavior change around from_none to make it easier to decide if those are a real problem or not.
I'm not yet sure that this is better enough than using #[darling(bound = "...")] on the deriving struct/enum to justify the added complexity.
Does serde attempt to do this?
| @@ -172,9 +172,48 @@ impl<'a> ToTokens for CheckMissing<'a> { | |||
| let ty = self.0.ty; | |||
| let name_in_attr = &self.0.name_in_attr; | |||
|
|
|||
| let custom_from_none = self.0.post_transform.map(|post_transform| { | |||
| let (transformer_ret, handle_error) = match &*post_transform.transformer.to_string() | |||
There was a problem hiding this comment.
This match feels very suspicious; it feels like some of the logic should be moved to methods of the PostfixTransform struct, and if we now need the transform to be exactly one of these two functions we should change the type of PostfixTransform::transformer to an enum rather than allowing any Ident.
| quote! { | ||
| fn __from_none<__Fn, __Arg, __Ret>( | ||
| __function: __Fn, | ||
| __errors: &mut ::darling::error::Accumulator, | ||
| ) -> ::darling::export::Option<__Ret> | ||
| where | ||
| __Fn: ::darling::export::FnOnce(__Arg) -> #transformer_ret, | ||
| __Arg: ::darling::FromMeta, | ||
| { | ||
| let __ret = <__Arg as ::darling::FromMeta>::from_none().map(__function); | ||
| #handle_error | ||
| __ret | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
This seems like a behavior change with a bigger impact than it first suggests; right now, the map and and_then calls are only done if a value was present; doing them in absent conditions could produce very subtle behavior changes.
|
There's currently no As far as I know, I had some time to think about the change, and the way I implemented it is definitely not backwards compatible. Not only, because the transform functions are called in case of a missing value, but mainly, because this also changes the It should be possible to implement this change without changing the current behaviour using autoref specialization. As a result, the Alternatively (or additionally), it should also be possible to instead drop the Would love to hear your thoughts about this. |
|
I'd be interested in seeing a version of the Also, have you considered adding a |
|
I’ve decided that I really like the composability of |
Maybe there's already a way of doing that, that I missed.
If a field is transformed using
maporand_thenit would be nice if there's noFromMetabound on the field type, but only on the argument type of the transform function.This is not totally straightforward, since it's not yet possible to directly name the argument/return type of a function. But it's possible using a dedicated function and type inference.
I'm not sure if this fits the coding style of the rest of the repo. I'm happy to change it.
The code is formatted using
rustfmt 1.5.1-nightly (215e3cd 2022-11-03).