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

Make Value be From<Option<T>> #900

Merged
merged 2 commits into from
Jun 29, 2022

Conversation

kvnvelasco
Copy link
Contributor

@kvnvelasco kvnvelasco commented Jun 24, 2022

Adds an implementation for From<Option<T>> for all T where T: Into<Value>

Use case: Ergonomics for manually constructing values for insertion into BTreeMap (sentry does this)

I have no idea where / how the tests happen. If this is an acceptable change, could someone point me in the direction of the tests?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Could you share how this would improve sentry's use case? What would they be writing without and with this impl?

@kvnvelasco
Copy link
Contributor Author

kvnvelasco commented Jun 25, 2022

So sentry has a context type you can attach to errors BTreeMap<String, Value>.

Almost every instance where context needs to be added to a sentry event, the value is derived from other pieces of state. Most of the time they are Option<T>

where

let user_meta: Option<String> = None;
context.insert(
  "user_meta".into(), 
   match user_meta { 
      Some(u) => u.into(),
      None => Value::Null
   }
)

Is a pretty common expression. We don't really want to have to bring in a macro everywhere (because there are probably hundreds of these).

All other non option types we send take advantage of the From implementations.

With the impl it'd be

context.insert(
  "user_meta".into(), 
   user_meta.into()
)

Also as opposed to

context.insert(
  "user_meta".into(), 
   to_value(user_meta).unwrap()
)

Also, not (at) sentry. My bad. I meant when using sentry.

Copy link
Member

@dtolnay dtolnay 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 clarification. This looks good to me.

@dtolnay dtolnay merged commit df704c2 into serde-rs:master Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants