Conversation
Workaround for limitations in `#[xml(deserialize_with = "..")]` when the field, i.e. `Option<Duration>`, does not already have a `FromXml` implementation to borrow `Accumulator` and `matches()` from.
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
I want to remove the various
Fromimplementations forCreateDataand simply
makes the fields public. I removed the need to create additional constructors
for aUpdateData.
It seems okay to make CreateData::data and both of the DsOrKey fields public.
The commit history in this PR feels fairly fragmented, the individual commit messages don't make it all that clear how they contribute to the overal goal of supporting the info command. Suggest flattening at least to one commit per type with new trait impls.
| pub(crate) data: T, | ||
| } | ||
|
|
||
| macro_rules! from_scalar { |
There was a problem hiding this comment.
Can we do this with generics/a wrapper type (potentially inside instant-xml) instead of a macro?
| Self { | ||
| data: DsOrKeyType { | ||
| maximum_signature_lifetime: Some(maximum_signature_lifetime), | ||
| maximum_signature_lifetime: Some(MaximumSignatureLifeTime( |
There was a problem hiding this comment.
Can we fix the root cause instead of working around it here?
There was a problem hiding this comment.
I would like that too. I tried at first with deseserialize_with, but these need to be specified to:
fn matches(id: Id<'_>, _: Option<Id<'_>>) -> bool {
id == instant_xml::Id {
ns: XMLNS,
name: Self::ELEMENT_NAME,
}
}
type Accumulator = Option<Self>;
const KIND: instant_xml::Kind = instant_xml::Kind::Scalar;Perhaps some similar to serdes remote could be used? Set a #[xml(remote = MaximumSignatureLifeTime)] on the field and require a impl Into<Duration> for MaximumSignatureLifeTime
| } | ||
|
|
||
| #[derive(Debug, ToXml)] | ||
| #[derive(Debug, Clone, ToXml, FromXml)] |
There was a problem hiding this comment.
I think this is the same thing we discussed yesterday, but not still not fully clear on why these need Clone -- it seems unintuitive.
There was a problem hiding this comment.
Yeah, same reason. It's either that or changing it to a Vec.
2 participants
One step closer to fixing #17
Draft because it relies on the unreleased changes made in djc/instant-xml#70. 😊I want to remove the various
Fromimplementations forCreateDataand simplymakes the fields public. I removed the need to create additional constructors
for a
UpdateData.Complex
Fromsignatures and implementations that serve only as "constructors"are in my opinion an antipattern. I don't think it's a sustainable interface to
be consistent with for the remaining commands.