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

Add ActiveValue::try_as_ref() #2197

Merged
merged 1 commit into from
May 9, 2024
Merged

Conversation

Expurple
Copy link
Contributor

Another helper method that I use very often and would like to upstream. Basically, a non-panicking version of ActiveValue::as_ref.

PR Info

  • Closes

  • Dependencies:

  • Dependents:

New Features

  • ActiveValue::try_as_ref()

Bug Fixes

Breaking Changes

Changes

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 18, 2024

Thanks. But is try_as_ref the best name? I am trying to find something similar in other Rust libraries.

@Expurple
Copy link
Contributor Author

Expurple commented Apr 18, 2024

I think, it's a good name because it's consistent with the existing as_ref, and there's also a third party try_as_traits::TryAsRef::try_as_ref for using this pattern generically. I've decided against adding an extra dependency because I don't need to be abstract & generic, and inherent methods are more discoverable.

In my dependent codebase, this helper is called ActiveValueExt::value. But when I was upstreaming it, I discovered that this name would be confusing in combination with existing into_value and into_wrapped_value which return Value rather than V.

Overall, it's up to you. I'm ok with other names, as long as they don't contain value. E.g. as_ref (if it wasn't already taken), borrow (you may not like it because it sounds like Borrow::borrow but has a different signature), get...

For other examples in the ecosystem, you can check out the methods on async_graphql::MaybeUndefined. It's a similar Option-like enum

@billy1624
Copy link
Member

Hey @Expurple, thanks for contributing!!

Maybe value or as_value? We already have unwrap, into_value and into_wrapped_value methods.

/// Get an owned value of the [ActiveValue]
///
/// # Panics
///
/// Panics if it is [ActiveValue::NotSet]
pub fn unwrap(self) -> V {
match self {
ActiveValue::Set(value) | ActiveValue::Unchanged(value) => value,
ActiveValue::NotSet => panic!("Cannot unwrap ActiveValue::NotSet"),
}
}
/// Check if a [Value] exists or not
pub fn into_value(self) -> Option<Value> {
match self {
ActiveValue::Set(value) | ActiveValue::Unchanged(value) => Some(value.into()),
ActiveValue::NotSet => None,
}
}
/// Wrap the [Value] into a `ActiveValue<Value>`
pub fn into_wrapped_value(self) -> ActiveValue<Value> {
match self {
Self::Set(value) => ActiveValue::set(value.into()),
Self::Unchanged(value) => ActiveValue::unchanged(value.into()),
Self::NotSet => ActiveValue::not_set(),
}
}

@Expurple
Copy link
Contributor Author

Just choose any one name that you agree to merge, then I make the edit and you merge it. The final approve decision is up to you anyway. I've already listed some options and explained why I prefer try_as_ref/borrow/get to value/as_value, but all of these will do the job and I'm not interested in bikeshedding

@billy1624
Copy link
Member

Hey @tyt2y3, perhaps value / as_value?

Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the explanation!

@tyt2y3 tyt2y3 merged commit e162a1b into SeaQL:master May 9, 2024
32 checks passed
Copy link

🎉 Released In 1.0.0-rc.5 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

Copy link

github-actions bot commented Aug 4, 2024

🎉 Released In 1.0.0 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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

Successfully merging this pull request may close these issues.

3 participants