gh-rust-lang/log
Watch
1
0
Fork
You've already forked log
 0
Code Issues 16 Pull requests 2 Releases Activity

kv macro support #353

Merged
yoshuawuyts merged 10 commits from kv-support into master 2019-12-09 00:00:45 -06:00
Conversation 20 Commits 10 Files changed 4 +115 -4
yoshuawuyts commented 2019-09-01 11:42:39 -05:00 (Migrated from github.com)

Adds key-value support to the log macros. Supersedes #346. Refs #343 #328. Thanks!

Syntax

femme::start(log::LevelFilter::Info).unwrap();
info!("hello");
info!("hello",);
info!("hello {}", "cats");
info!("hello {}", "cats",);
info!("hello {}", "cats", {
    cat_1: "chashu",
    cat_2: "nori",
});

Tasks

  • write tests
Adds key-value support to the log macros. Supersedes #346. Refs #343 #328. Thanks! ## Syntax ```rust femme::start(log::LevelFilter::Info).unwrap(); info!("hello"); info!("hello",); info!("hello {}", "cats"); info!("hello {}", "cats",); info!("hello {}", "cats", { cat_1: "chashu", cat_2: "nori", }); ``` ## Tasks - [ ] write tests
KodrAus (Migrated from github.com) reviewed 2019-09-01 16:29:38 -05:00
KodrAus (Migrated from github.com) left a comment

Thanks so much for working on this @yoshuawuyts!

I've just left a comment about how we're capturing values as strings.

Thanks so much for working on this @yoshuawuyts! I've just left a comment about how we're capturing values as strings.
src/lib.rs
@ -1388,3 +1389,4 @@
args: fmt::Arguments,
level: Level,
&(target, module_path, file, line): &(&str, &'static str, &'static str, u32),
kvs: Option<&[(&str, &str)]>,
KodrAus (Migrated from github.com) commented 2019-09-01 16:29:07 -05:00

We should use &ToValue here so that the structure of captured values can be retained. I sketched out the change in https://github.com/yoshuawuyts/kv-log-macro/pull/1

We should use `&ToValue` here so that the structure of captured values can be retained. I sketched out the change in https://github.com/yoshuawuyts/kv-log-macro/pull/1
KodrAus commented 2019-09-01 17:41:28 -05:00 (Migrated from github.com)

Ah, we should also be careful not to regress the info!("Hello, {value}", value = 42); case, just based on kv-log-macro it looks like our changes to the macro syntax may not support the ident = expr syntax?

It does also present a bit of a syntactic oddity, where values interpolated into the message would use the ident = expr syntax from format_args, but the captured key-value pairs use the ident: expr syntax like structs:

info!("hello {value}", value = "cats", {
    cat_1: "chashu",
    cat_2: "nori",
});

I don't think this is necessarily a problem though, but thought it was worth pointing out.

Ah, we should also be careful not to regress the `info!("Hello, {value}", value = 42);` case, just based on [`kv-log-macro`](https://github.com/yoshuawuyts/kv-log-macro) it looks like our changes to the macro syntax may not support the `ident = expr` syntax? It does also present a bit of a syntactic oddity, where values interpolated into the message would use the `ident = expr` syntax from `format_args`, but the captured key-value pairs use the `ident: expr` syntax like structs: ```rust info!("hello {value}", value = "cats", { cat_1: "chashu", cat_2: "nori", }); ``` I don't think this is necessarily a problem though, but thought it was worth pointing out.
yoshuawuyts (Migrated from github.com) reviewed 2019-09-01 18:26:14 -05:00
src/lib.rs
@ -1388,3 +1389,4 @@
args: fmt::Arguments,
level: Level,
&(target, module_path, file, line): &(&str, &'static str, &'static str, u32),
kvs: Option<&[(&str, &str)]>,
yoshuawuyts (Migrated from github.com) commented 2019-09-01 18:26:14 -05:00

Ohhhh, yesss! -- I wish I'd known about this sooner, haha.

Ohhhh, yesss! -- I wish I'd known about this sooner, haha.
yoshuawuyts commented 2019-09-01 18:27:00 -05:00 (Migrated from github.com)

@KodrAus in general it seems we don't have a lot of tests right now -- I was wondering how we might want to go about it?

@KodrAus in general it seems we don't have a lot of tests right now -- I was wondering how we might want to go about it?
KodrAus commented 2019-09-01 18:43:40 -05:00 (Migrated from github.com)

we don't have a lot of tests right now

Yeh, we don't really for the macro syntax, do we. Maybe we could add an integration test file that can do something like this:

struct AssertRecord;

impl Log for AssertRecord {
    fn record(&self, record: &Record) {
        ASSERT.with(|a| a(record));
    }
}

#[test]
fn level_info() {
    // Sets the assert function in thread-local storage
    // then calls the log statement
    test_log!(
        info!("Hello, {value}!", value = "world"),
        |record| {
            assert_eq!("Hello, world!", record.args().to_string();
            assert_eq!(Level::Info, record.level());
        }
    );
}

So that we can build up a suite for the current syntax.

But I'm happy to sketch that out independently if you like? For now I think if we can try find a way to keep ident = expr interpolated values working then we should be good!

> we don't have a lot of tests right now Yeh, we don't really for the macro syntax, do we. Maybe we could add an integration test file that can do something like this: ```rust struct AssertRecord; impl Log for AssertRecord { fn record(&self, record: &Record) { ASSERT.with(|a| a(record)); } } #[test] fn level_info() { // Sets the assert function in thread-local storage // then calls the log statement test_log!( info!("Hello, {value}!", value = "world"), |record| { assert_eq!("Hello, world!", record.args().to_string(); assert_eq!(Level::Info, record.level()); } ); } ``` So that we can build up a suite for the current syntax. But I'm happy to sketch that out independently if you like? For now I think if we can try find a way to keep `ident = expr` interpolated values working then we _should_ be good!
yoshuawuyts commented 2019-09-01 19:07:05 -05:00 (Migrated from github.com)

@KodrAus ohhh, yeah looks like you were onto something there -- I found a bug in my impl which I've fixed, but more importantly the name = val interpolated args aren't indeed no longer working. I've added some basic tests for the macros to prevent this from regressing.

Though I'm not entirely sure how to fix it, so I'm all ears if you have suggestions!

@KodrAus ohhh, yeah looks like you were onto something there -- I found a bug in my impl which I've fixed, but more importantly the `name = val` interpolated args aren't indeed no longer working. I've added some basic tests for the macros to prevent this from regressing. Though I'm not entirely sure how to fix it, so I'm all ears if you have suggestions!
KodrAus commented 2019-09-11 22:00:18 -05:00 (Migrated from github.com)

Hi @yoshuawuyts!

I'm just circling back to this, I'll spend some time playing with the macros too and see where I end up. We might end up having to tweak the syntax a little (which we could do to consider working more nicely with other extensions like #357).

Hi @yoshuawuyts! I'm just circling back to this, I'll spend some time playing with the macros too and see where I end up. We might end up having to tweak the syntax a little (which we could do to consider working more nicely with other extensions like #357).
yoshuawuyts commented 2019-09-12 05:10:25 -05:00 (Migrated from github.com)

@KodrAus Hi! -- I've been meaning to post an update, but kept putting it off because of reasons, but yea I've spent some time working on this though haven't been able to solve it.

So what seems to be happening is that the macro currently correctly desugars into:

format_args!("hello {cat}", cat = "nori");

2019-09-12-120638_1920x1080

But the output isn't working. Why? Well, it turns out that format_args doesn't know what to do with the types, and doesn't further process them but expands them to literally stay as cat="nori", and then tries to match on them:

2019-09-12-120857_1920x1080

Now what I suspect is happening here is that cat="nori" is not the right token type, and format_args doesn't know how to process it. This is where I got stuck, because I don't know how to cast the types in macro_rules macros.

I've tried processing it as a tt stream. And also asserted it's indeed an ident (which according to astexplorer.net is the expected type. So well, yeah, that's as far as I got.

Hope this was helpful!

@KodrAus Hi! -- I've been meaning to post an update, but kept putting it off _because of reasons_, but yea I've spent some time working on this though haven't been able to solve it. So what seems to be happening is that the macro currently correctly desugars into: ```rust format_args!("hello {cat}", cat = "nori"); ``` ![2019-09-12-120638_1920x1080](https://user-images.githubusercontent.com/2467194/64775124-c117a700-d555-11e9-98a6-9889f9af8bae.png) But the output isn't working. Why? Well, it turns out that `format_args` doesn't know what to do with the types, and doesn't further process them but expands them to literally stay as `cat="nori"`, and then tries to match on them: ![2019-09-12-120857_1920x1080](https://user-images.githubusercontent.com/2467194/64775229-f58b6300-d555-11e9-816b-e1663a719224.png) Now what I suspect is happening here is that `cat="nori"` is not the right token type, and `format_args` doesn't know how to process it. This is where I got stuck, because I don't know how to cast the types in `macro_rules` macros. I've tried processing it as a `tt` stream. And also asserted it's indeed an `ident` (which according to [astexplorer.net](https://astexplorer.net) is the expected type. So well, yeah, that's as far as I got. Hope this was helpful!
yoshuawuyts commented 2019-11-01 06:20:14 -05:00 (Migrated from github.com)

@KodrAus did you have any luck getting this to work? If not, perhaps we could ask @dtolnay for help?

@KodrAus did you have any luck getting this to work? If not, perhaps we could ask @dtolnay for help?
KodrAus commented 2019-11-04 15:39:22 -06:00 (Migrated from github.com)

Hi @yoshuawuyts! 👋

Sorry I’ve been really scattered over the last few months so haven’t been able to make a lot of progress here. However I’m feeling less concerned about regressing this specific edge-case, and I also think implicit inline format args makes the difference between our binding syntax for format args vs kv pairs much less significant.

So all that’s to say I’d be on board with pushing forward with this!

Hi @yoshuawuyts! :wave: Sorry I’ve been really scattered over the last few months so haven’t been able to make a lot of progress here. However I’m feeling less concerned about regressing this specific edge-case, and I also think [implicit inline format args](https://github.com/rust-lang/rfcs/pull/2795) makes the difference between our binding syntax for format args vs kv pairs much less significant. So all that’s to say I’d be on board with pushing forward with this!
❤️ 1
yoshuawuyts commented 2019-11-06 19:35:38 -06:00 (Migrated from github.com)

@KodrAus yay, that's excellent news! I'm very excited for this!

I just pushed a patch to remove the failing tests, meaning this patch should now be ready to be merged! It seems CI is currently not setup, so it can't verify this patch. But hopefully this is good!

@KodrAus yay, that's excellent news! I'm very excited for this! I just pushed a patch to remove the failing tests, meaning this patch should now be ready to be merged! It seems CI is currently not setup, so it can't verify this patch. But hopefully this is good!
KodrAus commented 2019-11-06 20:52:06 -06:00 (Migrated from github.com)

It seems CI is currently not setup

Oh dear... Lemme just fix that real quick.

> It seems CI is currently not setup Oh dear... Lemme just fix that real quick.
KodrAus commented 2019-11-09 03:31:19 -06:00 (Migrated from github.com)

@yoshuawuyts would you like to squash and rebase your commits on the current master? I'm thinking the reason actions aren't running here is because we're missing the .github/workflows directory on your branch.

@yoshuawuyts would you like to squash and rebase your commits on the current `master`? I'm thinking the reason actions aren't running here is because we're missing the `.github/workflows` directory on your branch.
yoshuawuyts commented 2019-11-09 04:23:11 -06:00 (Migrated from github.com)

@KodrAus rebased and force-pushed. Hoping this works! 🤞

@KodrAus rebased and force-pushed. Hoping this works! :crossed_fingers:
yoshuawuyts commented 2019-11-09 04:34:30 -06:00 (Migrated from github.com)

Heh, for odd reasons CI is no longer reporting on this branch. My guess is because the repo was migrated it caused some bug in GitHub's UI.

CI link is here: https://github.com/yoshuawuyts/log/runs/295531981 -- it seems cfg_if is failing to build?

Heh, for odd reasons CI is no longer reporting on this branch. My guess is because the repo was migrated it caused some bug in GitHub's UI. CI link is here: https://github.com/yoshuawuyts/log/runs/295531981 -- it seems `cfg_if` is failing to build?
KodrAus commented 2019-11-09 17:56:39 -06:00 (Migrated from github.com)

Ah it looks like cfg-if has removed their explicit MSRV, so I'm trying to get everything back into a good state in #362.

Ah it looks like `cfg-if` has removed their explicit MSRV, so I'm trying to get everything back into a good state in #362.
yoshuawuyts commented 2019-12-08 22:15:36 -06:00 (Migrated from github.com)

Not behind a computer right now but we should try running CI again. Looks like #362 got merged two weeks ago.

Not behind a computer right now but we should try running CI again. Looks like #362 got merged two weeks ago.
KodrAus (Migrated from github.com) approved these changes 2019-12-09 00:00:38 -06:00
KodrAus (Migrated from github.com) left a comment

Yes! Let's merge this in. CI should be good now but if there's any issues I'll get them sorted.

Thanks so much for your patience and effort on this one @yoshuawuyts!

Yes! Let's merge this in. CI should be good now but if there's any issues I'll get them sorted. Thanks so much for your patience and effort on this one @yoshuawuyts!
yoshuawuyts commented 2019-12-09 00:13:40 -06:00 (Migrated from github.com)

@kodraus Wooot! So happy this got merged! Thank you!

Do you have any idea when this could make it to a release on crates.io? I'm super stoked to start trying it out! ✨

@kodraus Wooot! So happy this got merged! Thank *you*! Do you have any idea when this could make it to a release on crates.io? I'm super stoked to start trying it out! ✨
KodrAus commented 2019-12-09 17:24:56 -06:00 (Migrated from github.com)

@yoshuawuyts Yeh this is an exciting step forward! I'll put together a release PR today and cc you on it. I might get a few other peeps from Libs to take a look at the diff too just to make sure we haven't missed anything.

@yoshuawuyts Yeh this is an exciting step forward! I'll put together a release PR today and cc you on it. I might get a few other peeps from Libs to take a look at the diff too just to make sure we haven't missed anything.
🎉 1
Sign in to join this conversation.
Reviewers
No reviewers
KodrAus
Labels
Clear labels   
bug

   
duplicate

   
easy

   
enhancement

   
help wanted

   
invalid

   
question

   
rfc

   
structured-logging

   
wontfix
No labels
bug
duplicate
easy
enhancement
help wanted
invalid
question
rfc
structured-logging
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-rust-lang/log!353
No description provided.