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

How should we implement Valuable for Uuid? #87

Open
KodrAus opened this issue Feb 22, 2022 · 3 comments
Open

How should we implement Valuable for Uuid? #87

KodrAus opened this issue Feb 22, 2022 · 3 comments

Comments

@KodrAus
Copy link

KodrAus commented Feb 22, 2022

Hi! 👋

cc @QnnOkabayashi

Over in uuid-rs/uuid#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 (dyn Display primitive value #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
Copy link
Author

KodrAus commented Feb 23, 2022

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
Copy link
Member

hawkw commented Feb 23, 2022

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
Copy link
Author

KodrAus commented Feb 27, 2022

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 🤔

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

No branches or pull requests

2 participants