NoneAsEmptyString: Deserialize using FromStr

[mkroening]

The docs for NoneAsEmptyString are wrong. For deserialization for<'a> From<&'a str> is used instead of FromStr.

In my case, only FromStr is available, so I have to use rust::string_empty_as_none instead. Those docs advertise identical functionality using NoneAsEmptyString, so that advertisement either needs to be removed or NoneAsEmptyStrings functionality adjusted.

Which would you prefer?

PS: Thanks for this awesome project! :)

[codecov]

Codecov Report

Merging #316 (b945c18) into master (e90d7f4) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   79.24%   79.39%   +0.15%     
==========================================
  Files          45       45              
  Lines        2587     2577      -10     
==========================================
- Hits         2050     2046       -4     
+ Misses        537      531       -6     

Impacted Files	Coverage Δ
tests/serde_as/lib.rs	100.00% <ø> (ø)
src/de/impls.rs	74.87% <100.00%> (+0.94%)	⬆️
src/rust.rs	74.14% <0.00%> (+2.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e90d7f4...b945c18. Read the comment docs.

[jonasbb]

Thanks for informing me about this mistake. I'm happy the crate is otherwise useful though :)

I'm actually unsure what the best fix here is. Honoring the documentation and keeping it consistent with rust::string_empty_as_none are both good reasons to change the implementation. On the other hand, this would remove support for types like Box<str>, Arc<str> which are tested and are probably the reason I changed the trait bounds in the first place.

The documentation is written mainly for Option<String>. There is no problem here as both From<&str> and FromStr work. On GitHub all uses of NoneAsEmptyString seem to be either inside this crate or for Option<String>, such that either change might be practically ok.

NoneAsEmptyString is not meant to work with arbitrary inner types but only string like ones, even if Box<str> etc. are not yet documented. So I wouldn't feel too bad to keep using From<&str> to enable Box<str>.

[mkroening]

Thanks for the quick reply and the research!

I'm working with Option<url::Url> but want to treat empty strings as None, so I was looking for a combination of DisplayFromStr and NoneAsEmptyString. I am not sure if working with Url is in scope for NoneAsEmptyString, if that one is not meant to work with arbitrary inner types.

If both behaviors are useful, we could just introduce a second type. I am not sure what would be a good name, maybe NoneAsEmptyDisplay? 🤔

On further thought, this might be possible to do in a modular fashion. I'm thinking about PickFirst<(NoneAsEmptyStr, Option<DisplayFromStr>)> with NoneAsEmptyStr mapping None as empty str and vice versa but failing otherwise. I'll try that solution later, or do you think this is not possible?

[jonasbb]

Oh, Url is interesting as it is string like. I think for now the better way is to change the implementation to use FromStr. If somebody needs support for Box<str> etc. this can be added as a separate type.

Using PickFirst<(NoneAsEmptyStr, Option<DisplayFromStr>)> should work for deserializing. Serializing like that won't work, as PickFirst always uses the first type for serializing. Or to be more specific: The SerializeAs implementation of your NoneAsEmptyStr type would need to do all the translating between the serialized string and the Option<Url>. At that point it is probably easier to copy the implementation of NoneAsEmptyString into your crate and change the trait bounds yourself.

[mkroening]

All right. I pushed the change to relay deserialization of NoneAsEmptyString to rust::string_empty_as_none.

Couldn't PickFirst be adjusted to try the second serialization option if the first one fails? Is this a design choice or is there some inherent limitation in trying to serialize that I am not aware of?

[jonasbb]

Thanks for updating the PR. I'm going to merge this as it and release a new bugfix version later.

bors r+

Regarding PickFirst: It is mainly a design decision to use the first variant. It is also documented this way "Serialization always picks the first option.".

Of course it is possible to create a type, which tries multiple serializing behaviors. It is much more uncommon for the serialization to fail though, so I am not sure how generally useful such a type would be. An example where serialization can fail are maps with non-string keys when using JSON, but this is caused by the Serializer not the Serialize implementation. Deserialize implementations on the other hand fail quite often, for example because a string contains an unexpected value.

I'm not sure it would work for your case though. The first argument, NoneAsEmptyStr would need to handle the Option<Url> is None case. So far that is ok. But the second argument, Option<DisplayFromStr> will serialize an optional string. serde_json chooses to not model Some entries and instead just use the inner value. But this does not hold for all Serializers. RON for example explicitly serializes the None and Some variants. Thus the end results would be that None → "" but Some("hello") → Some("hello").

[bors]

Build failed:

• Tests result

[jonasbb]

bors r+

[bors]

Build succeeded:

• Rustsec Audit
• Tests result

[mkroening]

Ah, I see. Thanks for explaining.

I just found this idea interesting to explore. For my use case I am happy with the behavior introduced by this PR though. Thanks a lot for your help! :)