Implement a #[jomini(collect_with)] attribute for JominiDeserialize. - #235
Open
jcranmer wants to merge 2 commits into
Open
Implement a #[jomini(collect_with)] attribute for JominiDeserialize.#235jcranmer wants to merge 2 commits into
jcranmer wants to merge 2 commits into
Conversation
nickbabcock
reviewed
Jan 20, 2026
nickbabcock
left a comment
nickbabcock
left a comment
Contributor
There was a problem hiding this comment.
Heck yeah, super excited!
| #match_arm => Ok(#field_ident) | ||
| } | ||
| }); | ||
| let field_enum_match2 = field_enum_match.clone(); |
Contributor
There was a problem hiding this comment.
Collecting into a vec and then using the same reference seems preferable.
Author
|
Sorry for the delay in responding to the review comments, I instead got busy, uh, playing the game instead. 😅 |
nickbabcock
reviewed
Mar 1, 2026
| /// | ||
| /// - `#[jomini(token=value)]` | ||
| /// | ||
| /// Annoytating all fields with this enables the binary deserializer to map `u16` tokens directly to |
Contributor
There was a problem hiding this comment.
Suggested change
| /// Annoytating all fields with this enables the binary deserializer to map `u16` tokens directly to | |
| /// Annotating all fields with this enables the binary deserializer to map `u16` tokens directly to |
| { | ||
| match __value { | ||
| #(#field_enum_match),* , | ||
| _ => Ok(__Field::__unknown_str(__value.into())), |
Contributor
There was a problem hiding this comment.
I'm not sure I know what the behavior of this should be for structs that don't have a collect_with, and I'm a little wary of every impl having this extra field and functionality not intended for it.
If there was a cleaner break in impls that have the collectd_with attribute and those without, I'd be comfortable merging this to minimize any potential downsides.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.