-
Notifications
You must be signed in to change notification settings - Fork 253
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
Support static string as key value #486
Conversation
@KodrAus this was a lot easier then I expected, so I'm a little worried I'm forgetting about a use case I'm breaking. |
@@ -30,7 +30,7 @@ | |||
#[macro_export(local_inner_macros)] | |||
macro_rules! log { | |||
// log!(target: "my_target", Level::Info; key1 = 42, key2 = true; "a {} event", "log"); | |||
(target: $target:expr, $lvl:expr, $($key:ident = $value:expr),+; $($arg:tt)+) => ({ | |||
(target: $target:expr, $lvl:expr, $($key:tt = $value:expr),+; $($arg:tt)+) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh unfortunately I think this will include quotes in string keys. So for "string/key"
the actual string we get will look like "\"string/key\""
.
Looking at this again though, I don't think we actually need a tt muncher yet afterall. We can just use another macro around __log_stringify!
(or just re-use __log_stringify!
for it) that matches on literals and idents and handles them appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this means the tests I've added are also flawed as they should have caught this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this in __log_key
, formerly __log_stringify
, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5daa3859e6f8a8e1245db028199687d9.
@Thomasdezeeuw I'll still push ahead with |
Just an update for you @Thomasdezeeuw. Turns out my permissions to the repo aren't fully sorted yet so I can't actually do any publishing. Just working to get that sorted so I think we probably will be able to fit this in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
Bumps [cargo_toml](https://gitlab.com/crates.rs/cargo_toml) from 0.12.4 to 0.13.0. - [Release notes](https://gitlab.com/crates.rs/cargo_toml/tags) - [Commits](https://gitlab.com/crates.rs/cargo_toml/commits/master) --- updated-dependencies: - dependency-name: cargo_toml dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes #484.