Implement tracing support with associated type on LogLevel trait #124

joshka · 2024-11-14T00:00:31Z

Rationale: #121 (comment)

src/lib.rs

examples/log_level.rs

coveralls · 2024-11-14T00:10:18Z

Pull Request Test Coverage Report for Build 11864002921

Details

41 of 77 (53.25%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-15.8%) to 51.648%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/tracing.rs	0	4	0.0%
src/lib.rs	21	37	56.76%
src/log.rs	20	36	55.56%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	1	52.94%

Totals
Change from base Build 11630869252:	-15.8%
Covered Lines:	47
Relevant Lines:	91

💛 - Coveralls

src/log.rs

epage · 2024-11-14T01:21:56Z

What seems logical to me for commits

Switch to a new type for VerbosityFilter (this is a pre-requisite for log being optional, no associated types used)
Make log optional
Add optional tracing support

joshka · 2024-11-14T01:32:15Z

Sounds reasonable. I started with the premise that it wasn't necessary as a wrapper on u8, but as an enum instead that maps onto LevelFilter it's pretty neat.

Ooh, idea. If instead of trying to convert from LevelFilter to Option I just add conversions from VerbosityFilter, then this is actually possible to acheive without that log PR.

epage · 2024-11-14T02:14:18Z

I started with the premise that it wasn't necessary as a wrapper on u8, but as an enum instead that maps onto LevelFilter it's pretty neat.

I'm fine with it being an enum instead of a u8.

Ooh, idea. If instead of trying to convert from LevelFilter to Option I just add conversions from VerbosityFilter, then this is actually possible to acheive without that log PR.

Not quite sure what you are referring to with this idea

joshka · 2024-11-14T02:26:35Z

Not quite sure what you are referring to with this idea

I wanted to move

clap-verbosity-flag/src/log.rs

Lines 6 to 13 in dddcca0

    
           impl<L: LogLevel<LevelFilter = LevelFilter>> Verbosity<L> { 
        
               /// Get the log level. 
        
               /// 
        
               /// `None` means all output is disabled. 
        
               pub fn log_level(&self) -> Option<Level> { 
        
                   self.log_level_filter().to_level() 
        
               } 
        
           }

(and the equivalent in tracing), into the main file rather than being log specific. I thought I might be able to do it by converting log::LevelFilter to Option<log::Level> and tracing_core::LevelFilter to Option<tracing_core::Level>, tracing implements a conversion trait, but log doesn't. The log PR I mentioned above addresses that, but it's not actually necessary. Instead I can add a type bound that converts from VerbosityFilter to Option<L::Level>, and implement that for the two crates. This would mean that the code in log.rs/tracing.rs is just simple conversion methods and log level implementations.

This makes the calculated log level filter more explicit by introducing a new `Filter` type that can be converted directly to and from the log Level and LevelFilter types.

- Add log feature flag - Move log specific code to log module - Add Level and LevelFilter associated types on LogLevel trait - Duplicate Verbosity struct with cfg flags as there's no easy way to have an optional default for an associated type BREAKING CHANGE: The log crate is now an optional dependency, enabled by default. The `LogLevel` trait now has associated types for `Level` and `LevelFilter` instead of these being the log crate types. The `log::Level` and `log::LevelFilter` types are now re-exported from the log module rather than the top-level module. This is a breaking change that will affect users who have implemented the `LogLevel` trait or imported the `Level` and `LevelFilter` types from the top-level module.

Add a new `tracing` feature flag and tracing module to support the `tracing` crate. Fixes: clap-rs#121

Previously the example code was using the `tracing-log` crate to integrate with the `tracing` ecosystem. This crate is no longer necessary as `tracing` now provides implementations of the `LogLevel` trait that expose the `tracing` log levels instead of the `log` log levels.

Reduces repetition and makes it easy to implement other levels

Fixes: clap-rs#122

joshka · 2024-11-14T07:48:42Z

I re-implemented this with smaller clean logical commits and a few changes to naming etc.

joshka · 2024-11-14T08:11:58Z

I don't grok why this error is showing up. If the default features are disabled, then the log and tracing modules should not be imported, and hence should not have any doc tests run. I'm guessing there's some weird edge-case for this that I'm missing the rationale for right now. Any ideas?

running `cargo test --no-default-features` on clap-verbosity-flag (1/6)
     Compiling clap-verbosity-flag v2.2.2 (/home/runner/work/clap-verbosity-flag/clap-verbosity-flag)
      Finished `test` profile [unoptimized + debuginfo] target(s) in 0.22s
       Running unittests src/lib.rs (target/debug/deps/clap_verbosity_flag-aa91d2ce2ac824dc)
  
  running 0 tests
  
  test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
     Doc-tests clap_verbosity_flag
  error[E0432]: unresolved import `log`
   --> src/log.rs:4:9
    |
  4 | pub use log::Level;
    |         ^^^ help: a similar path exists: `crate::log`
    |
    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
  
  error[E0432]: unresolved import `log`
   --> src/log.rs:5:9
    |
  5 | pub use log::LevelFilter;
    |         ^^^ help: a similar path exists: `crate::log`
    |
    = note: `use` statements changed in Rust 2018; read more at <https://doc.rust-lang.org/edition-guide/rust-2018/module-system/path-clarity.html>
  
  error[E0432]: unresolved import `tracing_core`
   --> src/tracing.rs:4:9
    |
  4 | pub use tracing_core::{Level, LevelFilter};
    |         ^^^^^^^^^^^^ use of undeclared crate or module `tracing_core`

joshka · 2024-11-14T08:23:55Z

ACK on the doc tests. Will take a look at these tomorrow / in a couple of days.

epage · 2024-11-14T15:40:30Z

src/lib.rs

+    }
+}
+
+impl fmt::Display for Filter {


Why do we have a Display impl?

Verbosity had a Display impl, which previously displayed the int value. In the associated type approach, Filter needed to be pub, so this kinda made sense, but in the crate specific methods approach it can be an implementation detail instead and so just making Verbosity's Display impl is the right approach.

6c0fef9

epage · 2024-11-14T15:45:01Z

src/lib.rs

 pub trait LogLevel {
+    type Level;
+    type LevelFilter;


no associated types used

With Filter, we shouldn't need any of this

It was either use associated types, or create crate specific methods (e.g. log_level, tracing_level). I think the specific methods are a much simpler approach.

7b8b965

- Change from using associated types and type constraints to a simpler approach using crate specific methods that return log and tracing types.

joshka · 2024-11-15T03:44:58Z

This PR subsumes #123 as it's simpler to just do that with the macroed impl.

I'm guessing that you probably would prefer the commits reordered to go straight to the multiple methods approach here?

epage · 2024-11-15T20:05:28Z

This PR subsumes #123 as it's simpler to just do that with the macroed impl.

Conditionally present trait methods on a non-sealed trait is an improper use of features because a feature can be enabled in another crate while I implement only the part of the trait I use. It could be argued that this crate isn't expected to be used in that way because this is for end-applications. However, a workspace could have a mixture of applications and a cargo check --workspace will unify them.

I had been under the assumption that the trait would return our custom filter type, making it independent of any of the logging interfaces, and not needing macros.

I would also prefer to handle these concerns in separate APis rather than merging them.

joshka · 2024-11-15T21:02:26Z

Conditionally present trait methods on a non-sealed trait is an improper use of features because a feature can be enabled in another crate while I implement only the part of the trait I use. It could be argued that this crate isn't expected to be used in that way because this is for end-applications. However, a workspace could have a mixture of applications and a cargo check --workspace will unify them.

Makes sense. I'll change that so that LogLevel::filter() returns Filter and make Filter public.

I had been under the assumption that the trait would return our custom filter type, making it independent of any of the logging interfaces, and not needing macros.

The macros aren't necessary, they just reduce the need for duplicate code to be written.

I would also prefer to handle these concerns in separate APis rather than merging them.

Can you clarify this - I'm not sure if by "separate APIs" you're agreeing with what I wrote above about having methods on Verbosity for each crate, or if you mean something else here.

- Expose the `Filter` type and use that instead of crate specific names. - Make Verbosity::filter() public, which allows just using `verbosity.filter().into()` in places that that makes sense

epage · 2024-11-15T21:16:41Z

Can you clarify this - I'm not sure if by "separate APIs" you're agreeing with what I wrote above about having methods on Verbosity for each crate, or if you mean something else here.

Sorry, I meant "separate PRs".

epage · 2024-11-15T21:17:41Z

Also, please cleanup the commit history for how you'd like it reviewed and merged. I know each maintainer on this is different but I find it difficult to review a PR when the commits go in different directions.

Moves the crate specific `impl Verbosity` blocks to the module rather than feature flagging each method Simplifies the tests so that the tests for which Filter is returned is in lib.rs rather than repeated, while the crate specific modules just check that the right conversion happens. Adds a derived Default impl to each of the LogLevel impls so that `Verbosity::default()` usefully compiles

joshka · 2024-11-15T21:53:56Z

Also, please cleanup the commit history for how you'd like it reviewed and merged. I know each maintainer on this is different but I find it difficult to review a PR when the commits go in different directions.

Yep - will do. Are you happy for me to do that once we've worked out the exact direction this ends up in?

src/lib.rs

joshka · 2024-11-15T21:16:03Z

src/lib.rs

-    fn default() -> Option<Level> {
-        Some(Level::Error)
-    }
+macro_rules! def_log_levels {


This saves about 40 lines of repetitive code. It's not strictly necessary, but I think that's a reasonable thing.

My bar for the use of Macros is high and I don't think this meets it.

My threshold for this is a bit lower than yours. IMO, if a macro save a reasonable amount of code that I'll have to read / maintain without adding too much complexity then it's worth including. This is because they make things that look similar actually be similar. Usually that's somewhere around 5 repetitions of 3 things (e.g. struct definition, associated derives, implementing a trait hits my threshold pretty niicely).

I'm happy to go with your threshold here, though I'd suggest giving some future thought about whether that could do with a slight adjustment. If not, no big deal :)

epage · 2024-11-15T21:55:29Z

Yep - will do. Are you happy for me to do that once we've worked out the exact direction this ends up in?

The other PR is pretty close to being merged, let's finish that up.

joshka · 2024-11-17T02:48:04Z

Split into 3 stacked PRs:

joshka commented Nov 14, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

joshka mentioned this pull request Nov 14, 2024

Tracing support #121

Open

github-advanced-security bot found potential problems Nov 14, 2024

View reviewed changes

examples/log_level.rs Fixed Show fixed Hide fixed

examples/log_level.rs Fixed Show fixed Hide fixed

joshka commented Nov 14, 2024

View reviewed changes

src/log.rs Outdated Show resolved Hide resolved

joshka force-pushed the jm/experiment-tracing branch from b137911 to dddcca0 Compare November 14, 2024 01:25

joshka force-pushed the jm/experiment-tracing branch 2 times, most recently from 6fd4d22 to 3e04897 Compare November 14, 2024 07:43

joshka added 6 commits November 13, 2024 23:45

refactor: Add internal Filter type

17d542d

This makes the calculated log level filter more explicit by introducing a new `Filter` type that can be converted directly to and from the log Level and LevelFilter types.

feat: Enable tracing support

e7be66a

Add a new `tracing` feature flag and tracing module to support the `tracing` crate. Fixes: clap-rs#121

refactor: Convert LogLevel impls into macro_rule

db29ee7

Reduces repetition and makes it easy to implement other levels

feat: Implement Off, Debug, and Trace levels

1b1eb91

Fixes: clap-rs#122

joshka force-pushed the jm/experiment-tracing branch from 3e04897 to 1b1eb91 Compare November 14, 2024 07:46

joshka force-pushed the jm/experiment-tracing branch from 8fd3009 to 684cd21 Compare November 14, 2024 08:08

fix: Various tests / lints

8caa1b8

joshka force-pushed the jm/experiment-tracing branch from 684cd21 to 8caa1b8 Compare November 14, 2024 08:12

epage reviewed Nov 14, 2024

View reviewed changes

joshka added 2 commits November 14, 2024 18:26

fix: Un-bifurcate the tracing impl

7b8b965

- Change from using associated types and type constraints to a simpler approach using crate specific methods that return log and tracing types.

fix: Make filter private and move Display impl to Verbosity

6c0fef9

joshka force-pushed the jm/experiment-tracing branch from 7d80f10 to 6c0fef9 Compare November 15, 2024 02:26

joshka marked this pull request as ready for review November 15, 2024 03:42

fix: make the default LogLevel use Filter

6cbe261

- Expose the `Filter` type and use that instead of crate specific names. - Make Verbosity::filter() public, which allows just using `verbosity.filter().into()` in places that that makes sense

joshka commented Nov 15, 2024

View reviewed changes

joshka mentioned this pull request Nov 17, 2024

feat(level): add off, debug, and trace options for default Level #95

Closed

joshka closed this Nov 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement tracing support with associated type on LogLevel trait #124

Implement tracing support with associated type on LogLevel trait #124

joshka commented Nov 14, 2024 •

edited

Loading

coveralls commented Nov 14, 2024 •

edited

Loading

epage commented Nov 14, 2024

joshka commented Nov 14, 2024

epage commented Nov 14, 2024

joshka commented Nov 14, 2024 •

edited

Loading

joshka commented Nov 14, 2024

joshka commented Nov 14, 2024

joshka commented Nov 14, 2024

epage Nov 14, 2024

joshka Nov 15, 2024

epage Nov 14, 2024

joshka Nov 15, 2024

joshka commented Nov 15, 2024 •

edited

Loading

epage commented Nov 15, 2024

joshka commented Nov 15, 2024

epage commented Nov 15, 2024

epage commented Nov 15, 2024

joshka commented Nov 15, 2024

joshka Nov 15, 2024

epage Nov 15, 2024

joshka Nov 17, 2024

epage commented Nov 15, 2024

joshka commented Nov 17, 2024

Implement tracing support with associated type on LogLevel trait #124

Implement tracing support with associated type on LogLevel trait #124

Conversation

joshka commented Nov 14, 2024 • edited Loading

coveralls commented Nov 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11864002921

Details

💛 - Coveralls

epage commented Nov 14, 2024

joshka commented Nov 14, 2024

epage commented Nov 14, 2024

joshka commented Nov 14, 2024 • edited Loading

joshka commented Nov 14, 2024

joshka commented Nov 14, 2024

joshka commented Nov 14, 2024

epage Nov 14, 2024

Choose a reason for hiding this comment

joshka Nov 15, 2024

Choose a reason for hiding this comment

epage Nov 14, 2024

Choose a reason for hiding this comment

joshka Nov 15, 2024

Choose a reason for hiding this comment

joshka commented Nov 15, 2024 • edited Loading

epage commented Nov 15, 2024

joshka commented Nov 15, 2024

epage commented Nov 15, 2024

epage commented Nov 15, 2024

joshka commented Nov 15, 2024

joshka Nov 15, 2024

Choose a reason for hiding this comment

epage Nov 15, 2024

Choose a reason for hiding this comment

joshka Nov 17, 2024

Choose a reason for hiding this comment

epage commented Nov 15, 2024

joshka commented Nov 17, 2024

joshka commented Nov 14, 2024 •

edited

Loading

coveralls commented Nov 14, 2024 •

edited

Loading

joshka commented Nov 14, 2024 •

edited

Loading

joshka commented Nov 15, 2024 •

edited

Loading