From e49f063235752784b0476e74559d142cd7786ca8 Mon Sep 17 00:00:00 2001
From: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
Date: Sun, 29 Aug 2021 14:01:02 +0200
Subject: [PATCH] Add key-values to the macros

Attempt number two/three? Too many in any case.

Previously I proposed a design that followed a `struct` like syntax:

```rust
info!("my message: {}", arg, {
    key1: "value1",
    key2: 123,
});
```

However it turns out that this does not work well with named arguments
as reported in issues #369 and #372. The implementation was eventually
reverted in pr #374.

This new design takes inspiration from the `tracing` crate which already
supports key-value pairs in logging events. The basic idea is to put the
key-value pairs before the message and arguments. Applying the same
structure like syntax as above we would get something like the
following.

```rust
info!({
    key1: "value1",
    key2: 123,
}, "my message: {}", arg);
```

But personally I'm not a big fan of this formatting, let's try putting
everything on a single line instead.

```rust
info!({ key1: "value1", key2: 123 }, "my message: {}", arg);
```

A little better, but at this point the structure like syntax is really
more annoying then helpful. So, instead I've done away it, opting
instead use the following syntax.

```rust
info!(key1 = "value1", key2 = 123, "my message: {}", arg);
```

Two major differences:
 * Removed the brackets.
 * Colons (`:`) are replaced with equal/assignment signs (`=`).

This gives us syntax similar to variable assignment.

But then we run in some limitations of the macro syntax, specifically
that `expr` fragments aren't allowed after `expr` fragments. To fix this
I went with the easiest option of changing the last comma (`,`) after
the key-value pairs to a semicolon (`;`). Making the final syntax look
like the following.

```rust
info!(key1 = "value1", key2 = 123; "my message: {}", arg);
info!(target: "my_target", key1 = "value1", key2 = 123; "my message: {}", arg);
log!(target: "my_target", log::Level::Info, key1 = "value1", key2 = 123; "my message: {}", arg);
```

Which, in my opinion and all things considered, it's too bad looking.
---
 src/lib.rs      | 30 ++++++++++++++++++++++++++++++
 src/macros.rs   | 20 ++++++++++++++++++++
 tests/macros.rs | 18 ++++++++++++++++++
 3 files changed, 68 insertions(+)

diff --git a/src/lib.rs b/src/lib.rs
index bbfb1ed31..6239594ea 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1452,10 +1452,39 @@ pub fn logger() -> &'static dyn Log {
 
 // WARNING: this is not part of the crate's public API and is subject to change at any time
 #[doc(hidden)]
+#[cfg(not(feature = "kv_unstable"))]
 pub fn __private_api_log(
     args: fmt::Arguments,
     level: Level,
     &(target, module_path, file, line): &(&str, &'static str, &'static str, u32),
+    kvs: Option<&[(&str, &str)]>,
+) {
+    if kvs.is_some() {
+        panic!(
+            "key-value support is experimental and must be enabled using the `kv_unstable` feature"
+        )
+    }
+
+    logger().log(
+        &Record::builder()
+            .args(args)
+            .level(level)
+            .target(target)
+            .module_path_static(Some(module_path))
+            .file_static(Some(file))
+            .line(Some(line))
+            .build(),
+    );
+}
+
+// WARNING: this is not part of the crate's public API and is subject to change at any time
+#[doc(hidden)]
+#[cfg(feature = "kv_unstable")]
+pub fn __private_api_log(
+    args: fmt::Arguments,
+    level: Level,
+    &(target, module_path, file, line): &(&str, &'static str, &'static str, u32),
+    kvs: Option<&[(&str, &dyn kv::ToValue)]>,
 ) {
     logger().log(
         &Record::builder()
@@ -1465,6 +1494,7 @@ pub fn __private_api_log(
             .module_path_static(Some(module_path))
             .file_static(Some(file))
             .line(Some(line))
+            .key_values(&kvs)
             .build(),
     );
 }
diff --git a/src/macros.rs b/src/macros.rs
index aed6fdeff..a90feef34 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -29,6 +29,17 @@
 /// ```
 #[macro_export(local_inner_macros)]
 macro_rules! log {
+    (target: $target:expr, $lvl:expr, $($key:ident = $value:expr),* ; $fmt:expr,  $($arg:tt)+) => ({
+        let lvl = $lvl;
+        if lvl <= $crate::STATIC_MAX_LEVEL && lvl <= $crate::max_level() {
+            $crate::__private_api_log(
+                __log_format_args!($fmt, $($arg)+),
+                lvl,
+                &($target, __log_module_path!(), __log_file!(), __log_line!()),
+                Some(&[$((__log_stringify!($key), &$value)),*])
+            );
+        }
+    });
     (target: $target:expr, $lvl:expr, $($arg:tt)+) => ({
         let lvl = $lvl;
         if lvl <= $crate::STATIC_MAX_LEVEL && lvl <= $crate::max_level() {
@@ -36,6 +47,7 @@ macro_rules! log {
                 __log_format_args!($($arg)+),
                 lvl,
                 &($target, __log_module_path!(), __log_file!(), __log_line!()),
+                None,
             );
         }
     });
@@ -248,3 +260,11 @@ macro_rules! __log_line {
         line!()
     };
 }
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! __log_stringify {
+    ($($args:tt)*) => {
+        stringify!($($args)*)
+    };
+}
diff --git a/tests/macros.rs b/tests/macros.rs
index 4daab32f9..0ccb64028 100644
--- a/tests/macros.rs
+++ b/tests/macros.rs
@@ -35,3 +35,21 @@ fn with_named_args() {
     info!("hello {cats}", cats = cats,);
     info!("hello {cats}", cats = cats,);
 }
+
+#[test]
+#[cfg(feature = "kv_unstable")]
+fn kv() {
+    info!(cat_1 = "chashu", cat_2 = "nori"; "hello {}", "cats");
+    info!(target: "my_target", cat_1 = "chashu", cat_2 = "nori"; "hello {}", "cats");
+    log!(target: "my_target", log::Level::Warn, cat_1 = "chashu", cat_2 = "nori"; "hello {}", "cats");
+}
+
+#[test]
+#[cfg(feature = "kv_unstable")]
+fn kv_expr_context() {
+    match "chashu" {
+        cat_1 => {
+            info!(target: "target", cat_1 = cat_1, cat_2 = "nori"; "hello {}", "cats")
+        }
+    };
+}