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

Review of key-value feature #584

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/kv/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ impl<'k> Key<'k> {
self.key
}

// REVIEW: do we still want this? Isn't most of log's API based on borrowed
Thomasdezeeuw marked this conversation as resolved.
Show resolved Hide resolved
// types?
/// Try get a string borrowed for the `'k` lifetime from this key.
pub fn to_borrowed_str(&self) -> Option<&'k str> {
// NOTE: This API leaves room for keys to be owned
Expand Down
6 changes: 6 additions & 0 deletions src/kv/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,12 @@ mod std_support {
}
}

// REVIEW: I'm not a 100% convinced that AsmAp and AsList are pulling their
Thomasdezeeuw marked this conversation as resolved.
Show resolved Hide resolved
// weight. Their Source implementation simply calls the underlying type's
// implementation, the only difference I can see is the sval::Value and
// serde::Serialize implementations (ignoring fmt::Debug). Does that fall within
// scope for the log crate?

/// The result of calling `Source::as_map`.
pub struct AsMap<S>(S);

Expand Down
14 changes: 14 additions & 0 deletions src/kv/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ macro_rules! as_sval {
/// - Using the `ToValue` trait.
/// - Using the standard `From` trait.
///
// REVIEW: maybe add a section about which conversion method to use and when? Or
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call 👍

The preference would be for capture_*, then from_*. The reason being if you Value::capture_display(&42) you'll get a number, but if you use Value::from_display(&42) you'll get a string. I'd be open to some different naming that made this more obvious. We may consider also only adding them for Display and Debug since serde and sval are naturally structured. We could also consider offering just a single capture method that is what capture_display currently is, and using that by default in the macros. The goal is that by default if you log a boolean or number you should get a boolean or number, and you shouldn't have to implement any non-standard-library traits like ToValue to log something.

I'm open to suggestions though. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does it make sense to change the macros to always call log::ToValue::to_value($value)? Then at least for the primitive types users don't have to worry about it. For non-primitive types users will have to use as_{debug,display,serde,sval}. Then, once specialisation is stablised (and log's MSRV is updated), we could even add impl<T: {fmt::Display,fmt::Debug,serde::Serialize,sval::Value}> ToValue for T { .. }.

As for the capture_* vs. from_*, can we drop the 'static requirement of the type? If maybe we can merge them (or at least some methods).

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to change the macros to always call log::ToValue::to_value($value)?

The idea was to try discourage libraries from implementing ToValue themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to Display + 'static means a lot of library types like Uuid can be captured, but the drawback of throwing lifetime errors at you if you try log a non-'static value would be unpleasant.

For what it's worth, I would be ok with using ToValue by default in addition to adding some easy syntax for Display/Debug/Serialize/Value. If we did that then we could remove the capture_* methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc.

Then, once specialisation is stablised

🥲

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does it make sense to change the macros to always call log::ToValue::to_value($value)?

The idea was to try discourage libraries from implementing ToValue themselves so it doesn't become yet another widespread piece of trivia that library authors need to keep on top of. Defaulting to Display + 'static means a lot of library types like Uuid can be captured, but the drawback of throwing lifetime errors at you if you try log a non-'static value would be unpleasant.

👍 makes sense

For what it's worth, I would be ok with using ToValue by default in addition to adding some easy syntax for Display/Debug/Serialize/Value. If we did that then we could remove the capture_* methods altogether; they only exist to support downcasting to tell if we're logging numbers/booleans etc.

We can keep using the as_display/as_debug macros, or shorten them to /?, but perhaps that a different discussion.

Then, once specialisation is stablised

🥲

Indeed.

// "the prefered ordering" of which methods to use. For example I think it's
// quite confusing when the use capture_* and when to use from_* (the only
// difference I can spot is the support for downcasting in capture_*).
///
/// ## Using the `Value::capture_*` methods
///
/// `Value` offers a few constructor methods that capture values of different kinds.
Expand Down Expand Up @@ -376,6 +381,9 @@ impl<'v> fmt::Display for Value<'v> {
}
}

// REVIEW: I'm wondering if people will accidentally use these implementations
Thomasdezeeuw marked this conversation as resolved.
Show resolved Hide resolved
// when converting a T: fmt::Debug/Display to a Value, losing the ability to
// downcast (without knowing it).
impl ToValue for dyn fmt::Debug {
fn to_value(&self) -> Value {
Value::from_dyn_debug(self)
Expand Down Expand Up @@ -615,6 +623,9 @@ mod std_support {

impl<'v> Value<'v> {
/// Try convert this value into a string.
// REVIEW: maybe rename this to contain COW somehow? I would expect &str
Thomasdezeeuw marked this conversation as resolved.
Show resolved Hide resolved
// be returned based on the to_str method name (same as e.g.
// Key::to_str).
pub fn to_str(&self) -> Option<Cow<str>> {
self.inner.to_str()
}
Expand All @@ -635,6 +646,9 @@ pub trait Visit<'v> {
/// more specific methods that aren't overridden.
/// The `Value` may be formatted using its `fmt::Debug` or `fmt::Display` implementation,
/// or serialized using its `sval::Value` or `serde::Serialize` implementation.
// REVIEW: Do we want seperate methods for sval and serde values? Otherwise
Thomasdezeeuw marked this conversation as resolved.
Show resolved Hide resolved
// we still have sort of "catch all" method in visit_any. Of course they can
// default to calling visit_any (as all other methods do).
fn visit_any(&mut self, value: Value) -> Result<(), Error>;

/// Visit an unsigned integer.
Expand Down