feat(martin-cp): indicativ based progress bar #2495
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
CommanderStorm
changed the title
There was a problem hiding this comment.
Pull request overview
This PR migrates the martin-cp tool from a custom progress bar implementation to the indicatif library. The changes integrate tracing-indicatif for progress bar support while maintaining tracing compatibility.
Changes:
- Adds
indicatifandtracing-indicatifdependencies to enable standardized progress bar functionality - Introduces
init_with_progressmethod in logging.rs to initialize tracing with indicatif integration - Refactors the
Progressstruct in martin-cp.rs to use indicatif'sProgressBarinstead of custom Display implementation - Extracts log bridge initialization into a separate
init_log_bridgefunction
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Adds workspace dependencies for indicatif 0.18.3 and tracing-indicatif 0.3 |
| Cargo.lock | Updates dependency lock with new indicatif-related packages and transitive dependencies |
| martin/Cargo.toml | References the new workspace dependencies for the martin crate |
| martin/src/logging.rs | Adds init_with_progress method and refactors init_tracing to support optional progress bar; extracts log bridge initialization |
| martin/src/bin/martin-cp.rs | Refactors Progress struct to use indicatif ProgressBar; removes custom duration formatting and Display implementation |
martin/src/logging.rs
Outdated
| // (e.g., by test harness or other initialization code) | ||
| let _ = log_builder.init(); |
There was a problem hiding this comment.
The refactored init_log_bridge function extracts the log bridge initialization logic, which is good for code organization. However, the comment on line 199-200 mentions avoiding panicking if the logger is already initialized, but the original code used .expect() which would panic. The new behavior silently ignores all errors including real initialization failures. Consider adding a debug log when initialization is skipped or fails for troubleshooting purposes.
| // (e.g., by test harness or other initialization code) | |
| let _ = log_builder.init(); | |
| // (e.g., by test harness or other initialization code), but log failures | |
| if let Err(err) = log_builder.init() { | |
| // This is usually benign (logger already initialized), but log for diagnostics. | |
| tracing::debug!("Skipping log bridge initialization: {err}"); | |
| } |
| pub fn init_with_progress(self, env_filter: EnvFilter) { | ||
| use tracing_subscriber::fmt::layer as fmt_layer; | ||
|
|
||
| let registry = tracing_subscriber::registry().with(env_filter.clone()); | ||
|
|
||
| // code below looks duplicated, but it has to be this way due to how types currently work. | ||
| // maybe there is a better way that I can not see | ||
| match self { | ||
| LogFormat::Full => { | ||
| let indicatif_layer = tracing_indicatif::IndicatifLayer::new(); | ||
| registry | ||
| .with( | ||
| fmt_layer() | ||
| .with_span_events(FmtSpan::NONE) | ||
| .with_writer(indicatif_layer.get_stderr_writer()), | ||
| ) | ||
| .with(indicatif_layer) | ||
| .init(); | ||
| } | ||
| LogFormat::Compact => { | ||
| let indicatif_layer = tracing_indicatif::IndicatifLayer::new(); | ||
| registry | ||
| .with( | ||
| fmt_layer() | ||
| .compact() | ||
| .with_span_events(FmtSpan::NONE) | ||
| .with_writer(indicatif_layer.get_stderr_writer()), | ||
| ) | ||
| .with(indicatif_layer) | ||
| .init(); | ||
| } | ||
| LogFormat::Pretty => { | ||
| let indicatif_layer = tracing_indicatif::IndicatifLayer::new(); | ||
| registry | ||
| .with( | ||
| fmt_layer() | ||
| .pretty() | ||
| .with_writer(indicatif_layer.get_stderr_writer()), | ||
| ) | ||
| .with(indicatif_layer) | ||
| .init(); | ||
| } | ||
| LogFormat::Bare => { | ||
| let indicatif_layer = tracing_indicatif::IndicatifLayer::new(); | ||
| registry | ||
| .with( | ||
| fmt_layer() | ||
| .compact() | ||
| .with_span_events(FmtSpan::NONE) | ||
| .without_time() | ||
| .with_target(false) | ||
| .with_ansi(false) | ||
| .with_writer(indicatif_layer.get_stderr_writer()), | ||
| ) | ||
| .with(indicatif_layer) | ||
| .init(); | ||
| } | ||
| LogFormat::Json => { | ||
| let indicatif_layer = tracing_indicatif::IndicatifLayer::new(); | ||
| registry | ||
| .with( | ||
| fmt_layer() | ||
| .json() | ||
| .with_span_events(FmtSpan::NONE) | ||
| .with_writer(indicatif_layer.get_stderr_writer()), | ||
| ) | ||
| .with(indicatif_layer) | ||
| .init(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The init_with_progress method contains significant code duplication with the original init method. The only differences are the creation of indicatif_layer and using get_stderr_writer(). Consider refactoring to reduce duplication, for example by extracting a helper method that builds the format layer with a writer parameter, or by having init delegate to init_with_progress with a conditional indicatif layer.
martin/src/logging.rs
Outdated
| // Use try_init to avoid panicking if logger is already initialized | ||
| // (e.g., by test harness or other initialization code) | ||
| let _ = log_builder.init(); |
There was a problem hiding this comment.
The error handling for the log bridge initialization has been changed from .expect("Failed to initialize log -> tracing bridge") to let _ = log_builder.init() which silently ignores errors. While the comment explains this is intentional to avoid panicking if already initialized, this could hide other initialization errors. Consider logging a debug message when initialization fails, or at least documenting in the function docstring that duplicate initialization attempts are silently ignored.
2 participants
This PR migrates us from our own progress bar to indicativ.
Needs tracing done first
This produces the following output:
and when done
I was resonably uncreative with the template, might be a better option, not sure -> https://docs.rs/indicatif/latest/indicatif/#templates
Also note that this is not the be-all/end-all of solutions, this for example has the following problems:
It does not use all of indicatif/indicatif-tracins' functionality -> for example we could use a trace or a multiprocess-style, ...
=> iterative development