Skip to content
This repository has been archived by the owner on Jun 9, 2024. It is now read-only.

Improve ergonomics #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Earthcomputer
Copy link
Collaborator

Based on my experience using valence_nbt in a large project, there are a few areas it could benefit from ergonomically.

This PR contains a couple of these:

  • Ability to convert a Value to an Option of one of its variants, which allows you take advantage of Option's methods, and in some cases avoid verbose pattern matching (although sometimes the pattern matching solution is the more elegant one).
  • Specialized methods in Compound to get a value of a specific type.
  • Typed Entrys for Compound. A common situation is that you want to look for an existing compound or list in another compound and add to it if it exists, or use a new one if it doesn't. This can now be done with compound.compound_entry("foo").or_default().insert("bar", 42);. Previously this required verbose pattern matching.

@Earthcomputer Earthcomputer requested a review from rj00a May 5, 2024 19:52
Copy link
Member

@rj00a rj00a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding the expanded API surface and macro code difficult to navigate. I feel there's a big opportunity to leverage traits to reduce the proliferation of macros here.

If we had TryFrom impls we could do the following instead of the various get_*, as_*, into_* methods. We're free to add as many TryFrom impls as we want without cluttering up the API.

impl Compound {
    // The return value can be driven by type inference, which is convenient.
    pub fn get<'a, T: TryFrom<&'a Value>>(&'a self, key: &str) -> Option<T> {
        T::try_from(self.map.get(key)?).ok()
    }

    pub fn get_mut<'a, T: TryFrom<&'a mut Value>>(&'a mut self, key: &str) -> Option<T> {
        T::try_from(self.map.get_mut(key)?).ok()
    }

    // ...
}

// TODO: Make this a proper error type.
pub struct TypeError;

// TODO: make it generic over the string type.
impl TryFrom<&'_ Value> for i32 {
    type Error = TypeError;

    fn try_from(value: &Value) -> Result<Self, Self::Error> {
        if let Value::Int(v) = value {
            // TODO: cast to an int like `Value::as_i32` does?
            Ok(*v)
        } else {
            Err(TypeError)
        }
    }
}

impl<'a> TryFrom<&'a Value> for &'a i32 {
    type Error = TypeError;

    fn try_from(value: &'a Value) -> Result<Self, Self::Error> {
        if let Value::Int(v) = value {
            Ok(v)
        } else {
            Err(TypeError)
        }
    }
}

impl<'a> TryFrom<&'a Value> for &'a Vec<i8> {
    type Error = TypeError;

    fn try_from(value: &'a Value) -> Result<Self, Self::Error> {
        match value {
            Value::ByteArray(v) => Ok(v),
            Value::List(List::Byte(v)) => Ok(v),
            _ => Err(TypeError),
        }
    }
}

impl<'a> TryFrom<&'a Value> for &'a [i8] {
    type Error = TypeError;

    fn try_from(value: &'a Value) -> Result<Self, Self::Error> {
        <&Vec<i8>>::try_from(value).map(|v| v.as_slice())
    }
}

^ Would be nice to do the same for List, Value, and friends.

I also think we should do something about this cumbersome trait bound

where
    Q: ?Sized + AsBorrowed<$string_type>,
    <Q as AsBorrowed<$string_type>>::Borrowed: Hash + Ord,
    $string_type: Borrow<<Q as AsBorrowed<$string_type>>::Borrowed>,

But that can be dealt with later.

Comment on lines +951 to +955
pub enum $name<'a, S = String> {
Vacant($vacant_name<'a, S>),
WrongType($wrong_type_name<'a, S>),
Occupied($occupied_name<'a, S>),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make generic Entry<T>, VacantEntry<T>, WrongTypeEntry<T>, OccupiedEntry<T> types to reduce the number of separate items we need to introduce?

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

Successfully merging this pull request may close these issues.

2 participants