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

Fix revert_to_default_in_memory #36

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

sgodwincs
Copy link
Contributor

I tried out the revert on deserialization error feature, but it doesn't handle reverting in memory completely resulting in the builder returning None.

The problem appears to be that default is being serialized as Option<R> instead of R, so deserialization fails with something like the following:

2024-01-10T09:22:01.462719Z ERROR bevy_persistent::format: failed to parse settings as pretty RON

1:5: Expected struct `Settings` but found `Some`
2024-01-10T09:22:01.462724Z ERROR bevy_persistent::persistent: failed to revert settings to default in memory due to a deserialization error
2024-01-10T09:22:01.462725Z ERROR bevy_persistent::persistent: failed to revert settings to default in memory

Also removed the last log statement since it seems redundant.

@umut-sahin
Copy link
Owner

Thanks for the PR @sgodwincs!

Indeed there was a bug, kinda embarrassing considering the rest of the library is well tested. It'd be perfect if you can add some tests to tests/persistent.rs, but if you don't have time, no worries, I can add them later.

Also, could you change the commit message to:

fix: serialize to R instead of Option<R> when reverting to default in memory

and maybe split the removed log to it's own commit with the message:

fix: stop logging the error twice when reverting to default in memory during initialization

Lastly, if you're not available, I can create another PR that solves this problem and make you the primary author of the relevant commits.

Let me know what do you want to do, and thanks again for reporting the issue and even fixing it 🙌

@sgodwincs
Copy link
Contributor Author

I've updated the commits and added some tests for revert_on_error.

@umut-sahin
Copy link
Owner

That's wonderful! I've started the CI, and I'll merge right away when it's green 👍

I'd love to add you to the list of authors before I release after merging this, could you give me your name and email address in this format:

authors = [
    "Umut Şahin <[email protected]>",
    "Shane Lillie <[email protected]>",
    "Eri Pazos <[email protected]>",
]

Thanks!

@umut-sahin umut-sahin merged commit 26feddc into umut-sahin:main Jan 11, 2024
7 checks passed
@umut-sahin
Copy link
Owner

I've used "Scott Godwin <[email protected]>" from your other projects.
And v0.4.3 is up on crates.io.
Thanks for the lovely contribution!

@sgodwincs
Copy link
Contributor Author

Yes, that works, thanks!

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.

2 participants