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

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

Closed
wants to merge 15 commits into from

Conversation

dekellum
Copy link
Contributor

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.

The `std::dbg!` macro from which this was inpsured, was only just
introduced in rust 1.32, but this just relies upon the `core` or `std`
`stringify` macro, which is "since 1.0.0"

Includes doctests and an external no-harness test.

Works with or without _std_, per testing.
@KodrAus
Copy link
Contributor

KodrAus commented Jan 20, 2019

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.

@dekellum
Copy link
Contributor Author

dekellum commented Jan 21, 2019

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 (An RFC for structured logging #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!

@KodrAus
Copy link
Contributor

KodrAus commented Jan 23, 2019

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
Copy link
Contributor

KodrAus commented Jan 23, 2019

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
Copy link
Contributor Author

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.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 24, 2019

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 dekellum changed the title Add pub debugv and tracev macros for inline expression logging Add pub "-v" macros for inline expression logging Jan 31, 2019
@dekellum
Copy link
Contributor Author

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
Copy link
Member

dtolnay commented Feb 16, 2019

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

@dekellum
Copy link
Contributor Author

dekellum commented Feb 16, 2019

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
Copy link
Contributor Author

dekellum commented Feb 17, 2019

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
Copy link
Member

dtolnay commented Feb 17, 2019

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.

src/lib.rs Outdated Show resolved Hide resolved
@dekellum
Copy link
Contributor Author

dekellum commented Mar 5, 2019

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.

@dekellum dekellum closed this Mar 5, 2019
EFanZh pushed a commit to EFanZh/log that referenced this pull request Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants