Add serde support#66
Conversation
chipsenkbeil
left a comment
chipsenkbeil
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I've read through and added comments on parts.
I think a big open question for me is how serde works with non-UTF8 bytes as I can see in part of the code the use of str::from_utf8 even when we are using Path<T>, PathBuf<T>, TypedPath, and TypedPathBuf which all support bytes that are not UTF-8.
Another thing of note is that the use of str::from_utf8 seems to be failing some tests because it's a 1.87.0 function, although there's some pre-existing function that's an alias to it. I don't want to bump this library that far up as there are other users of this project that are locked to older versions of Rust.
Final question is whether or not we can test without serde_json as it feels like some of the tests are json-specific, and I'd rather us verify the serde logic works and not be bolted to whether it works for serde_json.
| where | ||
| S: Serializer, | ||
| { | ||
| match self.to_str() { |
There was a problem hiding this comment.
We can visit bytes, but serializing has to convert to valid UTF-8 characters? Do we not support serializing to bytes?
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| deserializer.deserialize_str(PathVisitor(PhantomData)) |
There was a problem hiding this comment.
I don't know how serde works. This is deserializing into a string, but what about if we have a path that contains non-UTF8 bytes? The non-UTF8 path can have that.
| where | ||
| E: de::Error, | ||
| { | ||
| str::from_utf8(v) |
There was a problem hiding this comment.
Why are we converting to UTF8 here? I think this function is one of the newer ones that's causing the Rust version bump, but also our PathBuf type explicitly supports non-UTF8 characters within it.
| where | ||
| E: de::Error, | ||
| { | ||
| String::from_utf8(v) |
There was a problem hiding this comment.
Same question as earlier.
Why are we converting to UTF8 here? I think this function is one of the newer ones that's causing the Rust version bump, but also our PathBuf type explicitly supports non-UTF8 characters within it.
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| deserializer.deserialize_string(PathBufVisitor(PhantomData)) |
There was a problem hiding this comment.
Similar to Path, is this explicitly only supporting a path buf that can be converted to a string? I don't know enough about serde to answer myself. PathBuf supports non-UTF8 characters.
| #[test] | ||
| fn deserialize_root_windows_path() { | ||
| // Windows paths can't be borrowed from JSON since the backslashes need to be unescaped | ||
| assert!(serde_json::from_str::<TypedPath<'_>>(r#""\\some\\path\\to\\file.txt""#).is_err()); |
There was a problem hiding this comment.
Why are we testing JSON-specific limitations here? Can we do tests that just validate that serde serialization/deserialization works without being bolted to serde_json?
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| deserializer.deserialize_string(TypedPathBufVisitor) |
There was a problem hiding this comment.
Same question as others without me understanding serde: do we need to support byte types here alongside this string deserialize call? I don't know if serde offers that, but want to make sure that TypedPath and TypedPathBuf support both strings and bytes including non-UTF8 bytes.
| where | ||
| E: de::Error, | ||
| { | ||
| str::from_utf8(v) |
There was a problem hiding this comment.
Same comment as others, we support non-UTF8 bytes in TypedPathBuf, so I would expect that we derive directly from those bytes. Why are we forcing UTF-8 here?
| where | ||
| E: de::Error, | ||
| { | ||
| String::from_utf8(v) |
There was a problem hiding this comment.
Same comment as others, we support non-UTF8 bytes in TypedPathBuf, so I would expect that we derive directly from those bytes. Why are we forcing UTF-8 here?
| fn deserialize_root_windows_path() { | ||
| // Windows paths can't be borrowed from JSON since the backslashes need to be unescaped | ||
| assert!( | ||
| serde_json::from_str::<Utf8TypedPath<'_>>(r#""\\some\\path\\to\\file.txt""#).is_err() |
There was a problem hiding this comment.
Same comment elsewhere. This feels like we're testing serde_json and not how our serialize/deserialize works.
2 participants
This pull request adds support for serde gated behind a
serdefeature.This uses
serde_coreas recommended in order to be quick to compile since it doesn't rely on any proc macros and can compile in parallel withserde_derive.The actual implementations are effectively the same as what serde defines for
PathandPathBufin the standard library.Resolves #54.