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 defaults serialization and 'invalid type: unit value' deserialization error (#60) #106

Merged
merged 3 commits into from
May 9, 2019
Merged

Conversation

tyranron
Copy link
Contributor

@tyranron tyranron commented Apr 8, 2019

A second attempt to fix #60

Dupe of #95, but tries to follow the feedback.

Background

As @vorner wrote:

I think there are two issues here, aren't there?

First one is how to use defaults ‒ and the answer for that one is to use the serde's default handling (eg. the default attribute on fields).

The other problem is the annoying unit value error. I managed to work around that one by merging an empty source as the first one, eg config.merge(File::from_str("", FileFormat::Toml))?. Could something like this be already done as part of the constructor? I mean, make sure whatever is tried to be deserialized looks like an empty map to serde, not unit.

Solution

  1. The first issue is solved by providing two new methods:

    • set_defaults(&mut self, val: &T) -> Result<&mut Self>, where T: Serialize, which allows to mutate defaults values of Config by serializing them from given value.
    • try_defaults_from(val: &T) -> Result<Self>, where T: Serialize, which is the brother of try_from() but operates with defaults (not overrides).
  2. The second issue is solved by creating empty ValueKind::Table rather than ValueKind::Nil as @derekdreery pointed out.

Checklist

  • 1. Code style conformed.
  • 2. Tests are added.
  • 3. Documentation is added/corrected.

@tyranron
Copy link
Contributor Author

tyranron commented Apr 8, 2019

ping @mehcode

@mehcode mehcode merged commit c03fd36 into rust-cli:master May 9, 2019
mehcode added a commit that referenced this pull request May 9, 2019
@mehcode
Copy link
Collaborator

mehcode commented May 9, 2019

@tyranron Thank you very much for the fix. I decided to take out the additional methods ( pending the big rewrite in #111; don't want to increase the API surface ) but your added tests still pass if you add #[serde(default)] to your struct.

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.

When no configs are present, Config::new() should default to the structs default
2 participants