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

Auto Some-wrapping of Option<T> config values #231

Closed
Boscop opened this issue May 12, 2020 · 8 comments
Closed

Auto Some-wrapping of Option<T> config values #231

Boscop opened this issue May 12, 2020 · 8 comments

Comments

@Boscop
Copy link

Boscop commented May 12, 2020

E.g. if the Config struct contains a foo: Option<i32>, a value of foo: 42 in the config should be accepted, and converted to Some(42) automatically, instead of failing to deserialize.

@kvark
Copy link
Collaborator

kvark commented May 13, 2020

I believe this is what the newtype extension does - #72

@kvark kvark added the question label May 13, 2020
@Boscop
Copy link
Author

Boscop commented Jun 29, 2020

@kvark I don't mean newtype wrapping, I meant if my struct has a field #[serde(default)] foo: Option<i32> then currently the RON file has to contains foo: Some(42), even though if I want None, I just omit the field.
With serde_json, writing foo: 42 works, too (becomes Some(42)).

It could be said that's not a good behavior if there is ambiguity in cases like Option<Option<i32>>
but I think it doesn't have to be ambiguous, because if the RON file contains 42 it would unambiguously be Some(Some(42)) and if one wants to express Some(None), it would be written like that in the RON file.
The only ambiguous case is if the RON file contains None, but that can be said to mean None, not Some(None), because if Some(None) is meant, that should be written.
So the disambiguation rule would be to do the least interpretation work:
Deserializing None as None involves less interpretation than deserializing None as Some(None).

So it seems to be non-ambiguous, and it would really improve the usability of handwriting RON files to have auto-Some wrapping :)

@kvark
Copy link
Collaborator

kvark commented Jun 29, 2020

Ah sorry, you are totally right, discard my previous comment 😅

@jonasbb
Copy link

jonasbb commented Dec 14, 2020

@Boscop If you are willing to use third-party crates you could try serde_with::rust::unwrap_or_skip. It was written for RON. It only requires some attributes on the optional fields: #[serde(default, skip_serializing_if = "Option::is_none", with = "::serde_with::rust::unwrap_or_skip")]

@Boscop
Copy link
Author

Boscop commented Dec 15, 2020

@jonasbb Thanks. (Btw, this issue was about deserializing T into Option<T> which is now supported via the implicit_some extension.)
But yes, I also often want the reverse: Skipping None during serialization (and serializing only val instead of Some(val) in that case.
So the point of with = "::serde_with::rust::unwrap_or_skip" instead of just #[serde(default, skip_serializing_if = "Option::is_none")] is that Some(val) will serialize as just val, right?

@jonasbb
Copy link

jonasbb commented Dec 15, 2020

@Boscop The listed combination of attributes works for serialization and deserialization. This allows you to round trip the data. It also allows you to omit the Some(_) part without explicitly listing it as an extension.
Thanks, I didn't know about the extensions yet, I should check how implicit_some interacts with serde_with::rust::unwrap_or_skip.

So the point of with = "::serde_with::rust::unwrap_or_skip" instead of just #[serde(default, skip_serializing_if = "Option::is_none")] is that Some(val) will serialize as just val, right?

Yes, exactly. This then allows you to also serialize these documents.

@Boscop
Copy link
Author

Boscop commented Dec 15, 2020

It also allows you to omit the Some(_) part without explicitly listing it as an extension.

That's a great advantage, the extension attribute line confuses people who come from JSON.
I wish I could also enable the unwrap_newtypes extension statically (#281) so that it also works when serializing (#284).

@github-actions
Copy link

Issue has had no activity in the last 180 days and is going to be closed in 7 days if no further activity occurs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants