Refine ASR progress bar presentation - #13
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the console progress display by introducing a thin progress bar style, switching from raw counts to percentages, and improving time formatting. Feedback suggests that the manual newline handling in _stop_progress and the associated _stream_ends_with_newline helper are likely redundant and inefficient, as the Rich library typically manages terminal cleanup and getvalue() is not suitable for standard output streams.
| if not self._stream_ends_with_newline(): | ||
| self.stream.write("\n") | ||
| self.stream.flush() | ||
| self._last_width = 0 |
There was a problem hiding this comment.
The manual newline appended here appears redundant when using rich.progress.Progress with transient=False (the default in _ensure_progress), as Rich typically appends a newline when the live display stops. The current check _stream_ends_with_newline only works for streams that implement getvalue() (like io.StringIO used in tests) and will always return False for standard streams like sys.stdout. This will likely result in an extra blank line in real terminal output between files or steps. Consider removing this manual newline and relying on Rich's built-in cleanup.
self._last_width = 0| def _stream_ends_with_newline(self) -> bool: | ||
| getvalue = getattr(self.stream, "getvalue", None) | ||
| if not callable(getvalue): | ||
| return False | ||
| value = str(getvalue()) | ||
| return value.endswith("\n") |
There was a problem hiding this comment.
Using getvalue() on the stream to check for a trailing newline is inefficient for large streams, as it copies the entire buffer into memory. While this is primarily used in tests, it's a fragile pattern for a general-purpose observer. If the manual newline in _stop_progress is removed as suggested, this helper method would no longer be necessary.
|
Addressed Gemini review feedback in 8335300:
Verification:
|
Summary
Review
Test Plan