-
Notifications
You must be signed in to change notification settings - Fork 269
feat: enhance UpdateStats display with progress bar and error handling #1223
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
feat: enhance UpdateStats display with progress bar and error handling #1223
Conversation
PR #1223 Summary: Progress Bar Display with
|
|
@georgeh0 / @badmonster0 requesting review |
…in UpdateStats display
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.
Pull Request Overview
This pull request enhances statistics display with visual progress bars and better formatting, including TTY-aware colored output for terminal displays. It also adds a cumulative total tracker and suppresses dead code warnings for currently unused functions.
Key changes:
- Reimplemented the
UpdateStatsdisplay formatting with a visual progress bar and color-coded segments for different operation types (insertions, deletions, updates) - Added TTY detection to conditionally apply color formatting only when output is directed to a terminal
- Introduced a
cumulative_totalfield to track the net effect of operations over time
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/execution/stats.rs | Major refactoring of display logic to include progress bars, TTY detection, and cumulative totals tracking |
| src/ops/factory_bases.rs | Added #[allow(dead_code)] to suppress warning for take_value method |
| src/execution/db_tracking.rs | Added #[allow(dead_code)] to suppress warnings for read_source_state and upsert_source_state functions |
| examples/custom_output_files/run_and_capture.py | Added new Python helper script to run and capture output for testing |
| Cargo.toml | Added atty dependency for TTY detection |
| Cargo.lock | Updated lock file with new dependencies (atty, winapi, hermit-abi) and version adjustments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| messages.push(format!("{num_in_process} source rows IN PROCESS")); | ||
| // Error handling | ||
| if num_errors > 0 { | ||
| let tty = is_stdout_tty(); |
Copilot
AI
Nov 2, 2025
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.
The is_stdout_tty() function is called multiple times (lines 237, 300, 343) within the same fmt method. Consider calling it once at the beginning of the method and reusing the result to avoid redundant system calls.
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.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix for #343