Add pub "-v" macros for inline expression logging #316

2019-01-19T13:43:35-06:00

dekellum commented

2019-01-19 13:43:35 -06:00

(Migrated from github.com)

These new macro's were inspired by std::dbg! (just released in rust 1.32). Usage is the same, but these send output to the log instead of stderr, at the Debug or Trace level. Only std::stringify or core::stringify is required for the implementation, so this doesn't increase MSRV as these are "since 1.0.0".

Includes doctests and an external no-harness test.

Works with or without std, per testing.

These new macro's were inspired by `std::dbg!` (just released in rust 1.32). Usage is the same, but these send output to the log instead of stderr, at the `Debug` or `Trace` level. Only `std::stringify` or `core::stringify` is required for the implementation, so this doesn't increase MSRV as these are "since 1.0.0". Includes doctests and an external no-harness test. Works with or without _std_, per testing.

👍 3

KodrAus commented

2019-01-19 23:12:22 -06:00

(Migrated from github.com)

Hi @dekellum 👋

This is a neat idea! I'm not sure we need to provide it directly in log though, it's something that could be provided by a crate externally.

Hi @dekellum :wave: This is a neat idea! I'm not sure we need to provide it directly in `log` though, it's something that could be provided by a crate externally.

dekellum commented

2019-01-21 11:50:17 -06:00

(Migrated from github.com)

Edit: The below rationale is largely superseded by RFC #317.

Fair point. May I offer a rationale for including these macros here in log?

Background

The dbg! macro RFC: rust-lang/rfcs#2361.

In an executable project where configuring log and an output logger implementation hasn't (yet) happened, anyone can conveniently use std::dbg!, with no initial setup cost, for expression = value printing to stderr. From the linked rustdoc:

Note that the macro is intended as a debugging tool and therefore you should avoid having uses of it in version control for longer periods. Use cases involving debug output that should be added to version control may be better served by macros such as debug! from the log crate.

Indeed, a major point of the log package and Levels, is the ability to keep debug! and trace! logging in place for possible further use, including by other contributors, without paying
a cost for unlogged messages in release builds.

Its seems to follow that for executable projects that do already have log and an output logger dependency and configuration, with debug/trace logging already in place to help, that the use of std::dbg! would certainly be unwelcome in PRs and likely less productive than a log-based equivalent.

However, log doesn't currently have the same convenience macro(s) for logging expressions and passing through values inline.

Why add this to `log`?

The proposed debugv! and tracev! macros play a very similar role, for example to the existing debug! macro in log, which is just more convenient than using log!(Level::Debug, …) with an extra import.

Ease of use was an important part of the design and decision to add dbg! to rust std and the prelude.

Comment by the author of RFC 2361:

As to having this macro in a library, that makes it effectively worthless in my opinion. As already discussed in #2173, the whole point is having something that easier to use than println!("{:?}", foo), and println!("{:?}", foo) is easier than adding a dependency.

While the proposed log equivalent would still require an import, adding this feature to log avoids:

Needing to discover, add, and maintain an additional library dependency. Discovery I think would be the biggest issue, and the inclusion of dbg! in std and the prelude raises a high standard for parity.
The community effort to maintain such a separate library with compatibility to the log crate, as it evolves. This may sound trivial, but for example, I would think that debugv! and tracev! will need some work for parity with your structured logging work (#296). Its not clear to me, that this kind of external extension to the log interface has been factored into your design?

Alternative: cut-n’-paste macro reuse

As adding this feature to log might warrant a MINOR version bump, and/or you may want to take the time to further consider including this after more community feedback, a stop gap solution is available for those that want to try these convenience macros sooner in their own code base:

\\ -------------------------------- (cut here) ---------------------------------
\\ See: https://github.com/rust-lang-nursery/log/pull/316
macro_rules! debugv {
    ($val:expr) => {
        match $val {
            tmp => {
                debug!("{} = {:?}", stringify!($val), &tmp);
                tmp
            }
        }
    }
}
\\ -------------------------------- (cut here) ---------------------------------
\\ See: https://github.com/rust-lang-nursery/log/pull/316
macro_rules! tracev {
    ($val:expr) => {
        match $val {
            tmp => {
                trace!("{} = {:?}", stringify!($val), &tmp);
                tmp
            }
        }
    }
}
\\ -------------------------------- (cut here) ---------------------------------

Though I wouldn't recommend this, as it might cause more confusion than it avoids, debugv could even be locally named dbg to shadow the prelude std::dbg version.

I offer these reduced code snippets under the same permisive licenses as this PR's version and log! My inclination is to leave it at that, allowing you time to consider adding these to log directly. Thanks for your consideration and work on log!

**Edit: The below rationale is largely superseded by RFC #317.** Fair point. May I offer a rationale for including these macros here in `log`? ## Background The `dbg!` macro RFC: rust-lang/rfcs#2361. In an executable project where configuring `log` and an output logger implementation hasn't (yet) happened, anyone can conveniently use [`std::dbg!`](https://doc.rust-lang.org/std/macro.dbg.html), with no initial setup cost, for `expression = value` printing to stderr. From the linked rustdoc: > Note that the macro is intended as a debugging tool and therefore you should avoid having uses of it in version control for longer periods. Use cases involving debug output that should be added to version control may be better served by macros such as `debug!` from the `log` crate. Indeed, a major point of the `log` package and `Level`s, is the ability to keep `debug!` and `trace!` logging in place for possible further use, including by other contributors, without paying a cost for unlogged messages in release builds. Its seems to follow that for executable projects that _do already have_ `log` and an output logger dependency and configuration, with debug/trace logging already in place to help, that the use of `std::dbg!` would certainly be unwelcome in PRs and likely less productive than a `log`-based equivalent. However, `log` doesn't currently have the same convenience macro(s) for logging expressions and passing through values inline. ## Why add this to `log`? The proposed `debugv!` and `tracev!` macros play a very similar role, for example to the existing `debug!` macro in `log`, which is just more convenient than using `log!(Level::Debug, …)` with an extra import. Ease of use was an important part of the design and decision to add `dbg!` to rust `std` and the prelude. [Comment by the author of RFC 2361](https://github.com/rust-lang/rfcs/pull/2361#issuecomment-372732984): > As to having this macro in a library, that makes it effectively worthless in my opinion. As already discussed in #2173, the whole point is having something that easier to use than `println!("{:?}", foo)`, and `println!("{:?}", foo)` is easier than adding a dependency. While the proposed `log` equivalent would still require an import, adding this feature to `log` avoids: * Needing to _discover_, add, and maintain an additional library dependency. Discovery I think would be the biggest issue, and the inclusion of `dbg!` in `std` and the prelude raises a high standard for parity. * The community effort to maintain such a separate library with compatibility to the `log` crate, as it evolves. This may sound trivial, but for example, I would think that `debugv!` and `tracev!` will need some work for parity with your structured logging work (#296). Its not clear to me, that this kind of external extension to the `log` interface has been factored into your design? ## Alternative: cut-n’-paste macro reuse As adding this feature to `log` _might_ warrant a MINOR version bump, and/or you may want to take the time to further consider including this after more community feedback, a stop gap solution is available for those that want to try these convenience macros sooner in their own code base: ``` rust \\ -------------------------------- (cut here) --------------------------------- \\ See: https://github.com/rust-lang-nursery/log/pull/316 macro_rules! debugv { ($val:expr) => { match $val { tmp => { debug!("{} = {:?}", stringify!($val), &tmp); tmp } } } } \\ -------------------------------- (cut here) --------------------------------- \\ See: https://github.com/rust-lang-nursery/log/pull/316 macro_rules! tracev { ($val:expr) => { match $val { tmp => { trace!("{} = {:?}", stringify!($val), &tmp); tmp } } } } \\ -------------------------------- (cut here) --------------------------------- ``` Though I wouldn't recommend this, as it might cause more confusion than it avoids, `debugv` could even be locally named `dbg` to shadow the prelude `std::dbg` version. I offer these reduced code snippets under the same permisive licenses as this PR's version and `log`! My inclination is to leave it at that, allowing you time to consider adding these to `log` directly. Thanks for your consideration and work on `log`!

KodrAus commented

2019-01-23 16:43:04 -06:00

(Migrated from github.com)

Thanks for your thoughts @dekellum!

This is something that should be possible to support using structured logging (that would be preferable over interpolation into the message), however that would probably require some breakage.

I think the issue of discoverability is a little different for log than for std, because users have already committed to pulling in dependencies when they're using log. We could also make an effort to call out extensions of this kind in the readme, like we already do for frameworks.

Combined with additional freedom to iterate on the API (I can imagine requests for infov, warnv, errorv etc suggesting this feature should be somehow rolled into the existing log macros) and the fact that this is possible to build on top of log, I would suggest the external crate approach first.

Thanks for your thoughts @dekellum! This is something that should be possible to support using structured logging (that would be preferable over interpolation into the message), however that would probably require some breakage. I think the issue of discoverability is a little different for `log` than for `std`, because users have already committed to pulling in dependencies when they're using `log`. We could also make an effort to call out extensions of this kind in the readme, like we already do for frameworks. Combined with additional freedom to iterate on the API (I can imagine requests for `infov`, `warnv`, `errorv` etc suggesting this feature should be somehow rolled into the existing log macros) and the fact that this is possible to build on top of `log`, I would suggest the external crate approach first.

KodrAus commented

2019-01-23 17:07:16 -06:00

(Migrated from github.com)

Just a follow-up: I think your rationale is fair and thorough, and that this is something we should support somehow, I just feel like we need to give it time and space to iterate before committing to new API in log.

Just a follow-up: I think your rationale is fair and thorough, and that this is something we should support _somehow_, I just feel like we need to give it time and space to iterate before committing to new API in `log`.

dekellum commented

2019-01-23 18:07:38 -06:00

(Migrated from github.com)

I think its fair that you would want any public macro additions to be good and as stable as possible before adding to log. While I wanted to offer this PR sooner, as a proof-of-concept and to get it on the radar, I am reluctant to do the work for a new macro-only mini-crate, when the same or better version of this feature should later land in log!

Adding the remaining levels for the feature is reasonable, though it goes beyond strict parity with dbg!. That could be structured better with the bulk of the feature in one macro (like how log! is now).

Would you entertain a hopefully mini-RFC just focusing on the potential API additions? That discussion would be neutral to the log crate or another crate.

I think its fair that you would want any public macro additions to be good and as stable as possible before adding to `log`. While I wanted to offer this PR sooner, as a proof-of-concept and to get it on the radar, I am reluctant to do the work for a new macro-only mini-crate, when the same or better version of this feature should later land in `log`! Adding the remaining levels for the feature is reasonable, though it goes beyond strict parity with `dbg!`. That could be structured better with the bulk of the feature in one macro (like how `log!` is now). Would you entertain a hopefully mini-RFC just focusing on the potential API additions? That discussion would be neutral to the `log` crate or another crate.

❤️ 1

KodrAus commented

2019-01-23 19:18:17 -06:00

(Migrated from github.com)

Would you entertain a hopefully mini-RFC just focusing on the potential API additions? That discussion would be neutral to the log crate or another crate.

That sounds like a good idea!

> Would you entertain a hopefully mini-RFC just focusing on the potential API additions? That discussion would be neutral to the log crate or another crate. That sounds like a good idea!

dekellum commented

2019-02-16 09:02:07 -06:00

(Migrated from github.com)

Would someone in the know, like @dtolnay, be able to review the macro additions specifically?

I feel like I'm still a ~~macro amateur~~, err, an amateur at macros—I've learned much in the process, but I'm most certainly unpaid . Thanks in advance!

Would someone in the know, like @dtolnay, be able to review the macro additions specifically? I feel like I'm still a ~~macro amateur~~, err, an amateur *at macros*—I've learned much in the process, but I'm most certainly unpaid . Thanks in advance!

dtolnay commented

2019-02-16 13:02:16 -06:00

(Migrated from github.com)

I am an unpaid amateur too but this didn't work for me:

use log::{logv, Level};

fn main() {
    logv!(Level::Debug, 0);
}

error: cannot find macro `__logv!` in this scope
 --> src/main.rs:4:5
  |
4 |     logv!(Level::Debug, 0);
  |     ^^^^^^^^^^^^^^^^^^^^^^^ help: you could try the macro: `logv`
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

warning: unused import: `Level`
 --> src/main.rs:1:17
  |
1 | use log::{logv, Level};
  |                 ^^^^^
  |
  = note: #[warn(unused_imports)] on by default

I am an unpaid amateur too but this didn't work for me: ```rust use log::{logv, Level}; fn main() { logv!(Level::Debug, 0); } ``` ```console error: cannot find macro `__logv!` in this scope --> src/main.rs:4:5 | 4 | logv!(Level::Debug, 0); | ^^^^^^^^^^^^^^^^^^^^^^^ help: you could try the macro: `logv` | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) warning: unused import: `Level` --> src/main.rs:1:17 | 1 | use log::{logv, Level}; | ^^^^^ | = note: #[warn(unused_imports)] on by default ```

dekellum commented

2019-02-16 14:01:31 -06:00

(Migrated from github.com)

Oh boy, that's embarrassing as it seems like something my tests should have caught, since they do test logv!. Can you share any more details of your environment, rust version? Might the difference be that my tests (to be compatible with older rust), still use #[macro_use]? I must not understand something about the workound and comments you previously added for duel 2015/2018 edition support.

edit: I will write another 2018 specific test, in any case, thanks!

Oh boy, that's embarrassing as it seems like something my tests should have caught, since they do test `logv!`. Can you share any more details of your environment, rust version? Might the difference be that my tests (to be compatible with older rust), still use `#[macro_use]`? I must not understand something about the workound and comments you previously added for duel 2015/2018 edition support. edit: I will write another 2018 specific test, in any case, thanks!

dekellum commented

2019-02-16 18:48:58 -06:00

(Migrated from github.com)

It was simpler then that—I had just missed specifying the local_inner_macros on logv. (1) Sorry to divert your time with this, @dtolnay. Is there anything more generally concerning in how these macros are constructed?

One related item I'm not sure on is that, currently, the -v macros will refuse a trailing comma. Simple/nice handling of that would presumably be with rust-lang/rust#48075, e.g. $(,)? but that is 2018-edition only, and for rust 1.32+. With the limits of my knowledge, I'm inclined to leave that as is for now, with a comment, and awaiting a future MSRV 1.32 and 2018 version. Thoughts?

(1) I hadn't fully internalized that there are were no tests currently in log, that actually test 2018-edition style macro import. That could be fixed, likely best as a follow-on PR so as to include all existing public macros.

It was simpler then that—I had just missed specifying the `local_inner_macros` on `logv`. (1) Sorry to divert your time with this, @dtolnay. Is there anything more generally concerning in how these macros are constructed? One related item I'm not sure on is that, currently, the `-v` macros will refuse a trailing comma. Simple/nice handling of that would presumably be with rust-lang/rust#48075, e.g. `$(,)?` but that is 2018-edition only, and for rust 1.32+. With the limits of my knowledge, I'm inclined to leave that as is for now, with a comment, and awaiting a future MSRV 1.32 and 2018 version. Thoughts? (1) I hadn't fully internalized that there are were no tests currently in log, that actually *test* 2018-edition style macro import. That could be fixed, likely best as a follow-on PR so as to include all existing public macros.

dtolnay commented

2019-02-16 20:31:35 -06:00

(Migrated from github.com)

The implementation looks correct now. (Note: I only looked at the new macro_rules macros in src/macros.rs, not any of the other changes.)

I am not yet sold on adding all these macros. I left a comment on the RFC PR since that seems the more appropriate place for that discussion.

The implementation looks correct now. (Note: I only looked at the new macro_rules macros in src/macros.rs, not any of the other changes.) I am not yet sold on adding all these macros. I left a comment on the RFC PR since that seems the more appropriate place for that discussion.

dtolnay (Migrated from github.com) reviewed

2019-02-16 20:35:46 -06:00

src/lib.rs

					
				@ -15,36 +15,152 @@

				//! by this crate, and the consumer of those libraries can choose the logging

The type must implement Copy

Is this true? The following appears to compile even though String does not implement Copy:

let s = String::new();
let _: String = debugv!(s);

> The type must implement `Copy` Is this true? The following appears to compile even though String does not implement Copy: ```rust let s = String::new(); let _: String = debugv!(s); ```

dekellum (Migrated from github.com) reviewed

2019-02-18 12:10:01 -06:00

src/lib.rs

					
				@ -15,36 +15,152 @@

				//! by this crate, and the consumer of those libraries can choose the logging

Good catch, thanks, I'll change this to mention move semantics.

dekellum commented

2019-03-05 15:03:11 -06:00

(Migrated from github.com)

I'm going to close this for now, as it only implements an earlier version of RFC #317: the single, complete format parameter alternative. When and if RFC #317 is approved, a new PR can be submitted for a complete implementation.

Thanks for review feedback.

I'm going to close this for now, as it only implements an earlier version of RFC #317: the single, complete format parameter alternative. When and if RFC #317 is approved, a new PR can be submitted for a complete implementation. Thanks for review feedback.