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.

Sign up for GitHub

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 optional support for tracing besides log #37

Closed
wants to merge 1 commit into from
Closed

Add optional support for tracing besides log #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ codecov = { repository = "rust-cli/clap-verbosity-flag" }
[dependencies]
log = "0.4.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do tracing users generally feel about still requiring log? If a breaking change is involved in the future, should we allow disabling it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do tracing users generally feel about still requiring log?

I'm only a very new user of tracing, so I don't know, but I note that the main tracing README suggests that applications use the crate tracing_subscriber, which already, as a default feature, depends on log and consumes log messages, and the main tracing crate also interfaces with log as a non-default feature.

I'm sure some users would consider the cost in compilation time of log to be too much, but would they be using clap-derive?

clap = { version = "3.0", default-features = false, features = ["std", "derive"] }
tracing = { version = "0.1", default-features = false, optional = true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How mature / stable is tracing for being a 0.1 crate? We just went 1.0 and I don't want to repeatedly bump by taking on a potentially volatile dependency

Copy link
Author

@8573 8573 May 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How mature / stable is tracing for being a 0.1 crate?

I'm only a very new user of tracing, so I don't know. On one hand, I would guess that tracing::Level is too basic to be likely to disappear. On the other hand, in hindsight, considering the crates' very different version numbers, I do think it would be more appropriate for me to ask tracing to add the necessary conversion code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On one hand, I would guess that tracing::Level is too basic to be likely to disappear.

My concern is not with it disappearing but that when a tracing 0.2 comes out, to support it, we'd have to release clap-verbosity-flag 2.0. Ditto for each breaking release of tracing, regardless of whether the type looks the same. Rust will consider tracing::Level from 0.1, 0.2, 0.3, etc all to be different, incompatible types.


[dev-dependencies]
env_logger = "0.9.0"
23 changes: 21 additions & 2 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,22 @@ impl<L: LogLevel> Verbosity<L> {
}
}

/// Get the log level.
/// Get the log level, as defined by the `log` crate.
///
/// `None` means all output is disabled.
pub fn log_level(&self) -> Option<log::Level> {
level_enum(self.verbosity())
}

/// Get the log level filter.
/// Get the log level, as defined by the `tracing` crate.
///
/// `None` means all output is disabled.
#[cfg(feature = "tracing")]
pub fn tracing_level(&self) -> Option<tracing::Level> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tracing, is it more appropriate to return Level or LevelFilter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm not saying "also" but "instead of"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're very easily convertible to each other, and the equivalent of filter_level accepts both, so I don't think it matters much. I chose tracing::Level over tracing::level_filters::LevelFilter because I guessed that, if either's path were to change in the future, it more likely would be the latter's.

self.log_level().map(tracing_level_from_log_level)
}

/// Get the log level filter, as defined by the `log` crate.
pub fn log_level_filter(&self) -> log::LevelFilter {
level_enum(self.verbosity())
.map(|l| l.to_level_filter())
Expand Down Expand Up @@ -138,6 +146,17 @@ fn level_enum(verbosity: i8) -> Option<log::Level> {
}
}

#[cfg(feature = "tracing")]
fn tracing_level_from_log_level(level: log::Level) -> tracing::Level {
match level {
log::Level::Error => tracing::Level::ERROR,
log::Level::Warn => tracing::Level::WARN,
log::Level::Info => tracing::Level::INFO,
log::Level::Debug => tracing::Level::DEBUG,
log::Level::Trace => tracing::Level::TRACE,
}
}

use std::fmt;

impl<L: LogLevel> fmt::Display for Verbosity<L> {
Expand Down