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

Should We #[serde(deny_unknown_fields)]? #410

Closed
zicklag opened this issue Apr 5, 2022 · 1 comment · Fixed by #459
Closed

Should We #[serde(deny_unknown_fields)]? #410

zicklag opened this issue Apr 5, 2022 · 1 comment · Fixed by #459
Assignees
Milestone

Comments

@zicklag
Copy link
Member

zicklag commented Apr 5, 2022

I was wondering, does it make sense for us to deny unknown fields in our JSON assets?

It's a common source of confusion for me and I imagine other people when you get a typo in one of the field names and the game doesn't complain, so you think it worked, but in reality your new setting is being completely ignored.

I also found that the current JSON files have at least one particle_effect field in several files that isn't actually used in the struct anymore.

I'm not sure if we want to support older versions of files when possible, for backward compatibility, and allowing unknown fields helps that a little bit, but I think it would be better to do versioned asset files if we really wanted that.

@olefasting
Copy link
Member

I have been considering this, too.
Personally, I think it would be the best way to go, as it reduces confusion

@zicklag zicklag added this to the v0.5 milestone Jun 16, 2022
@zicklag zicklag self-assigned this Jun 16, 2022
@zicklag zicklag moved this from Todo to In Progress in @zicklag's FishFight Tasks Jun 16, 2022
@zicklag zicklag moved this from In Progress to Waiting For Merge in @zicklag's FishFight Tasks Jun 16, 2022
@zicklag zicklag mentioned this issue Jun 16, 2022
Repository owner moved this from Waiting For Merge to Done in @zicklag's FishFight Tasks Jun 30, 2022
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 a pull request may close this issue.

2 participants