Improve ActiveValue API

[billy1624]

Issue raised by @acidic9, thank you so much!!

The ActiveValue has some unwraps in it which might not be clear to the end user.

impl<V> ActiveValue<V> {
    pub fn take(&mut self) -> V {
        self.state = ActiveValueState::Unset;
        self.value.take().unwrap() // <- here, why unwrap instead of just returning the Option?
    }

    pub fn into_value(self) -> Value {
        self.value.unwrap().into() // <- here, why unwrap? Maybe .map is better
    }

    pub fn unwrap(self) -> V {
        self.value.unwrap()
    }
}

And the only way to get the inner value is actually to call .unwrap().
I think it'd make sense to make some changes:

pub fn take(&mut self) -> Option<V> {
    self.state = ActiveValueState::Unset;
    self.value.take() // <- dont upwrap here, instead return an Option to let the caller handle it
}

pub fn into_value(self) -> Option<Value> {
    self.value.map(Into::into) // <- no need to upwrap here also, just map and let caller unwrap if thats what they want
}

And also make the value itself public. So we can just call active_value.value (or have a method .inner() or something).
I think it'd be nice adding:

pub fn get_ref(&self) -> Option<&V> {
    self.value.as_ref()
}

pub fn into_inner(self) -> Option<V> {
    self.value
}

(copied the API from Actix's web::Data type) https://docs.rs/actix-web/3.3.2/actix_web/web/struct.Data.html#method.get_ref

[billy1624]

• I think we do have get_ref? But the return type is a bit different
  

  sea-orm/src/entity/active_model.rs

  Lines 234 to 241 in d85e66c

  	impl<V> std::convert::AsRef<V> for ActiveValue<V>
  	where
  	V: Into<Value>,
  	{
  	fn as_ref(&self) -> &V {
  	self.value.as_ref().unwrap()
  	}
  	}

• I guess we can take reference from the API of std::option::Option<T>. As ActiveValue<V> is essentially a stateful wrapper of Option<T>. Would this be a more natural way of using ActiveValue<V>?

[tqwewe]

Thank you much for making this issue @billy1624 .

Regarding this:

ActiveValue is essentially a stateful wrapper of Option

Option has a lot of methods on it, how do we decide which ones to copy and implement on ActiveValue?
Just a select few which might seem "important" (eg .unwrap(), .is_none(), .is_some(), ...)? All of them?

Or we could implement none of them, and instead just implement the methods get_ref() -> Option<&V> and into_inner() -> Option<V> which would let the user have access to the option directly.

[tyt2y3]

Some design thoughts, may be it's just me, but I always panic a bit when I saw Option<Option<T>>, reading it like "so it's an option of an option, where None is do nothing and Some(None) is set as null".

I think it is a good idea to have into_inner indeed.

[tyt2y3]

@acidic9 do you have any new thoughts on this matter?

[tqwewe]

Like I showed in my code snippet in the post by Billy, I think take can just return self.value.take(), and into_value can return self.value.map(Into::into), allowing the caller to handle the option and unwrap if they desire.