Implement a #[jomini(collect_with)] attribute for JominiDeserialize.#235
Implement a #[jomini(collect_with)] attribute for JominiDeserialize.#235jcranmer wants to merge 2 commits intorakaly:masterfrom
Conversation
nickbabcock
left a comment
nickbabcock
left a comment
There was a problem hiding this comment.
Heck yeah, super excited!
jomini_derive/src/lib.rs
Outdated
| /// | ||
| /// - `#[jomini(token=value)]` | ||
| /// | ||
| /// I don't know what this does. It needs documentation. |
There was a problem hiding this comment.
| /// I don't know what this does. It needs documentation. | |
| /// Annotating all fields with this enables the binary deserializer to map u16 tokens directly to struct fields, | |
| /// bypassing the intermediate string lookup and comparisons. |
| /// This attribute will discard any prior value of the field when it is | ||
| /// encountered. It is mutually exclusive with `#[jomini(duplicated)]`. | ||
| /// | ||
| /// - `#[jomini(token=value)]` |
There was a problem hiding this comment.
I believe it should be possible to use both token= and collect_with= (like in the case with EU4 trade node where the collected fields are strings -- and should not be interpreted as u16), so it'd be nice to have a test confirming this.
eu4 trade node reference where token= and collect_with= could be useful.
90969: id:0x15e={
90975: id:0x2835='african_great_lakes'
90981: id:0x2da5=f32:0x2092
91008: id:0x2aa8=f32:0x2092
[...]
91118: 'PIR'={
91133: id:0x2846=f32:0x3e8
91139: }
Can be left as a todo 🤔
| where __D: ::serde::Deserializer<'de> { | ||
| #[allow(non_camel_case_types)] | ||
| enum __Field { | ||
| enum __Field<'de> { |
There was a problem hiding this comment.
In #232 I mentioned how I was wary of potential performance impacts and thought it should be mitigated by emitting an "unknown"-aware Field and one that isn't based on the presence of collect_with.
I run some tests with eu4save, and there doesn't appear to be any performance impact, so I say we don't worry about it 😄
jomini_derive/src/lib.rs
Outdated
| #match_arm => Ok(#field_ident) | ||
| } | ||
| }); | ||
| let field_enum_match2 = field_enum_match.clone(); |
There was a problem hiding this comment.
Collecting into a vec and then using the same reference seems preferable.
| "MEE": 5 | ||
| }"#; | ||
|
|
||
| let m: Model = serde_json::from_str(data).unwrap(); |
There was a problem hiding this comment.
This test will fail when using serde_json::from_reader(data.as_bytes()), due to the fields being ephemeral strings
There was a problem hiding this comment.
Ephemeral strings can be fixed with use of Cow<'a, str> instead of &'a str I think, but I would expect performance implications from that.
There was a problem hiding this comment.
Yeah Cow sounds good, so perhaps we should only emit the Cow-based __Field when collect_with is present.
|
Sorry for the delay in responding to the review comments, I instead got busy, uh, playing the game instead. 😅 |
2 participants
This fixes issue #232, although I can't exactly guarantee that performance will be as good as it was before.
I've done some local testing with melted saves via a modified
eu4save, using this for collecting trade node data. I haven't done any testing with binary saves, so I can't confirm whether or not it works there.