iddm / serde-aux

An auxiliary serde library providing helpful functions for serialisation and deserialisation for containers, struct fields and others.
MIT License
152 stars 26 forks source link

Proposal: `deserialize_with_tilde` and `deserialize_optional_with_tilde` #24

Closed woodruffw closed 2 years ago

woodruffw commented 2 years ago

First of all, thanks for this library! It's very neat, and I think it's a great idea to collect Serde "recipes" like this.

Here are two more I propose:

The behavior here would come from the shellexpand crate, which only substitutes ~ if it's the first character in the string and the string is in "path" form, not "symbolic user" form (i.e. ~/foo/, not ~user/foo).

The main application of these is places where Serde is used to load from a config file: users generally prefer to specify paths with ~ rather than their home directory spelled out, and this would save a separate transform after deserialization.

Let me know what you think!

woodruffw commented 2 years ago

Oh, and for reference, here's how I've implemented these in the past (open to feedback):

#[doc(hidden)]
#[inline]
fn deserialize_with_tilde<'de, D>(deserializer: D) -> std::result::Result<String, D::Error>
where
    D: de::Deserializer<'de>,
{
    let unexpanded: &str = Deserialize::deserialize(deserializer)?;
    Ok(shellexpand::tilde(unexpanded).into_owned())
}

#[doc(hidden)]
#[inline]
fn deserialize_optional_with_tilde<'de, D>(
    deserializer: D,
) -> std::result::Result<Option<String>, D::Error>
where
    D: de::Deserializer<'de>,
{
    let unexpanded: Option<&str> = Deserialize::deserialize(deserializer)?;

    match unexpanded {
        Some(unexpanded) => Ok(Some(shellexpand::tilde(unexpanded).into_owned())),
        None => Ok(None),
    }
}
iddm commented 2 years ago

First, forgive me for replying so late. Thanks for the kind words about the crate!

Regarding your suggestion, here are some of my thoughts:

  1. The shellexpand crate isn't useful in my humble opinion. It just does nothing except for expanding in a string, it doesn't even use the PathBuf or Path and one will have to convert to it. Paths in strings in rust on their own are useless, one will always have to convert from a string into one of these types.
  2. In contrast to that, deserialisation into Path or PathBuf would have been much more useful in my opinion.
  3. I would go with the implementation for PathBuf and Path which, besides just parsing a path string, also canonicalizes the path or does more useful things before that.
  4. Even if it is done so, I don't think it is always useful to have an always-expanded path string. Besides, it is just a single function call away, it shouldn't be hard to do and isn't tedious, there is absolutely no magic of serde involved in that. Can we in principle do this? Yes, we can. But should we implement a "helper" function to deserialise which does nothing more than just a single function call?

I am sorry to disappoint! Let me know if you still think it is useful to have here.

And about your solution, yes, it is just the way I'd do the same.

woodruffw commented 2 years ago

Thanks for the detailed response! That's very fair; I don't think this is worth a PR in light of the feedback, so I'll close this tracking issue.