tokio-rs / valuable

MIT License
185 stars 19 forks source link

How should we implement Valuable for Uuid? #87

Open KodrAus opened 2 years ago

KodrAus commented 2 years ago

Hi! :wave:

cc @QnnOkabayashi

Over in https://github.com/uuid-rs/uuid/issues/583 we've been talking about adding unstable valuable support to Uuid and run into a bit of a design issue I wanted to get some input on.

It looks like we've got two possible directions we can go in:

  1. Treat Uuid as its text-based format and use Value::Displayable (#86). This means end-users in the diagnostics use-case will see something resembling a UUID rather than a blob of bytes without needing to match on strings (assuming tracing will eventually run all values through valuable), but it means reconstructing a Uuid from that dyn Display will require more work.
  2. Treat Uuid as its raw byte format and use Value::Listable. This is less work than 1, and a more accurate representation of the UUIDs internal structure, but if we want a text-based UUID we need to look at its stringly-typed name and parse it.

This seems like a general question that authors of primitive value types like UUIDs and IP addresses are going to need to answer, so was looking for some input on what you all think is the direction that best aligns with the goals of the valuable project.

KodrAus commented 2 years ago

On the one hand it seems like we could actually implement Valuable in terms of the underlying bytes as Listable, and also implement Displayable, except for these constraints on the subtypes of Valuable:

Values that implement Listable must return Value::Listable from their Valuable::as_value implementation.

It almost seems like something like this on the Valuable trait might make it possible to advertise multiple representations:

pub trait Valuable {
    fn as_value(&self) -> Value;

    // default impl that can be overridden
    fn as_listable(&self) -> Option<&dyn Listable> {
        self.as_value().as_listable()
    }

    ..
}

And remove the single subtrait constraint, supporting a default, and then any number of specialized alternative representations. That constraint might be there for a good reason though.

hawkw commented 2 years ago

On the one hand it seems like we could actually implement Valuable in terms of the underlying bytes as Listable,

This also makes me wonder a bit if there should be some kind of "ByteArrayable" primitive...

KodrAus commented 2 years ago

A &[u8] variant for Value seems reasonable to me, depending on how purely structural you're wanting to keep the Value enum. It does potentially mean someone looking for an array of bytes might need to consider both the Bytes and the Listable variants unless you require that values should produce the most specific variant possible (so a value that doesn't produce a Bytes variant is at fault and needs to be fixed). That would make it harder to add new specific variants in a semantically backwards-compatible way though 🤔