Add key-values to the macros #461

Merged

Thomasdezeeuw merged 1 commit from key-value-macros into master

2021-11-05 07:07:55 -05:00

Thomasdezeeuw commented

2021-08-29 07:21:37 -05:00

(Migrated from github.com)

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

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

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.

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.

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.

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.

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.

/cc @KodrAus @yoshuawuyts

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. /cc @KodrAus @yoshuawuyts

👍 1 ❤️ 1

Thomasdezeeuw commented

2021-09-03 10:49:20 -05:00

(Migrated from github.com)

Ping @KodrAus.

Thomasdezeeuw commented

2021-09-25 03:55:43 -05:00

(Migrated from github.com)

@sfackler you have the time to look at this?

KodrAus commented

2021-10-26 05:03:48 -05:00

(Migrated from github.com)

Hey @Thomasdezeeuw! 👋 Sorry I hadn't gotten to this. My GitHub notifications have been a mess lately. I don't currently have commit access to log but can still review in the meantime.

We are a bit limited in what we can do with macro_rules, I think the place you're arriving at is not too bad indeed. The key = value syntax is more in line with format_args, but a little inconsistent with target. If anything though, I'd consider target the inconsistent one.

This looks good to me 👍

Hey @Thomasdezeeuw! :wave: Sorry I hadn't gotten to this. My GitHub notifications have been a mess lately. I don't currently have commit access to `log` but can still review in the meantime. We are a bit limited in what we can do with `macro_rules`, I think the place you're arriving at is not too bad indeed. The `key = value` syntax is more in line with `format_args`, but a little inconsistent with `target`. If anything though, I'd consider `target` the inconsistent one. This looks good to me :+1:

👍 1

KodrAus (Migrated from github.com) approved these changes

2021-10-26 05:09:03 -05:00