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

kv macro support #353

Open
wants to merge 9 commits into
base: master
from

Conversation

@yoshuawuyts
Copy link
Contributor

commented Sep 1, 2019

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
yoshuawuyts added 4 commits Sep 1, 2019
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
fix tests
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Copy link
Contributor

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.

src/lib.rs Show resolved Hide resolved
@KodrAus KodrAus referenced this pull request Sep 1, 2019
6 of 25 tasks complete
@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

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.

@yoshuawuyts yoshuawuyts force-pushed the yoshuawuyts:kv-support branch from fe1b13e to 6cd5063 Sep 1, 2019
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

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!

yoshuawuyts added 4 commits Sep 2, 2019
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

@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!

@Thomasdezeeuw Thomasdezeeuw referenced this pull request Sep 11, 2019
@KodrAus

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@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!

bors bot added a commit to async-rs/async-std that referenced this pull request Sep 16, 2019
Merge #199
199: remove custom log code in favor of macro crate r=stjepang a=yoshuawuyts

This removes our custom log macro code in favor of using [`kv-log-macro`](https://github.com/yoshuawuyts/kv-log-macro). This is a temporary crate that exists only until rust-lang-nursery/log#353 lands which enables progress on rust-lang-nursery/log#328. Thanks!

Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
bors bot added a commit to async-rs/async-std that referenced this pull request Sep 16, 2019
Merge #199
199: remove custom log code in favor of macro crate r=yoshuawuyts a=yoshuawuyts

This removes our custom log macro code in favor of using [`kv-log-macro`](https://github.com/yoshuawuyts/kv-log-macro). This is a temporary crate that exists only until rust-lang-nursery/log#353 lands which enables progress on rust-lang-nursery/log#328. Thanks!

Co-authored-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.