-
Notifications
You must be signed in to change notification settings - Fork 258
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
Implement conversions between Option<Level> and LevelFilter #644
base: master
Are you sure you want to change the base?
Conversation
This is nice to have when working with the `log` crate, as it allows for more ergonomic conversions between the two types. Specifically, This allows type bounds that require Into<LevelFilter> or Into<Option<Level>> to be used. This in turn makes it possible to write more generic code that can work with tracing and log crates interchangeably. Specifically, this supports some ideas in clap-rs/clap-verbosity-flag#121
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While From<LevelFilter> for Option<Level>
is trivial and straightforward, None
to LevelFilter::Off
may not. I suppose different system may have different opinion for the "default" level filter.
You may write:
|
fn from(level: Option<Level>) -> LevelFilter { | ||
match level { | ||
Some(level) => level.to_level_filter(), | ||
None => LevelFilter::Off, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure this is always going to be obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you leave out "not" or are you disagreeing with the comment above here and saying that this is actually fine and a reasonable choice to convert Option<Level>::None
to LevelFilter::Off
?
Is None indicating a default, or the absence of a level? The benefit of making None -> Off is that it round trips. Perhaps a doc comment about this would be sufficient.
There's no |
BTW, I solved the underlying need for this through another means (changed the trait associated type bounds), so I'm unblocked from an implementation perspective for now at least. |
This is nice to have when working with the
log
crate, as it allowsfor more ergonomic conversions between the two types. Specifically, This
allows type bounds that require Into or Into<Option>
to be used. This in turn makes it possible to write more generic code
that can work with tracing and log crates interchangeably.
Specifically, this supports some ideas in
clap-rs/clap-verbosity-flag#121