[#259] Progress indicators for all operations #285
Conversation
mikejmorgan-ai
added
enhancement
WalkthroughThe PR introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention during review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # Demo 3: Progress bar | ||
| print("\n3. Progress bar:") | ||
| packages = ["nginx", "redis", "postgresql", "nodejs", "python3"] | ||
| for pkg in progress.progress_bar(packages, "Installing packages"): |
| import sys | ||
| import time | ||
| import threading | ||
| from typing import Optional, Callable, List, Dict, Any, Iterator |
| from rich.progress import ( | ||
| Progress, | ||
| SpinnerColumn, | ||
| TextColumn, | ||
| BarColumn, | ||
| TaskProgressColumn, | ||
| TimeElapsedColumn, | ||
| TimeRemainingColumn, | ||
| MofNCompleteColumn, | ||
| DownloadColumn, | ||
| TransferSpeedColumn | ||
| ) |
| DownloadColumn, | ||
| TransferSpeedColumn | ||
| ) | ||
| from rich.live import Live |
| TransferSpeedColumn | ||
| ) | ||
| from rich.live import Live | ||
| from rich.panel import Panel |
| from rich.live import Live | ||
| from rich.panel import Panel | ||
| from rich.table import Table | ||
| from rich.text import Text |
|
|
||
| import pytest | ||
| import time | ||
| import io |
| import pytest | ||
| import time | ||
| import io | ||
| import sys |
| import io | ||
| import sys | ||
| from datetime import datetime | ||
| from unittest.mock import Mock, patch, MagicMock |
| yield RichOperationHandle(self, context, icon) | ||
| else: | ||
| yield FallbackOperationHandle(self, context) | ||
| except Exception as e: |
There was a problem hiding this comment.
As a general rule, we should use the variable "e" to print the generic error. But in this case, since we are raising the error again (see line 232), it is OK to remove the unused variable as suggested by SonarCloud.
| """Update the progress message.""" | ||
| self._progress.update(f"{self.context.title} - {message}") | ||
|
|
||
| def log(self, message: str, style: str = ""): |
There was a problem hiding this comment.
Removing the unused param "style" is fine.
| assert step.name == "Test" | ||
| assert step.description == "Test step" | ||
| assert step.status == "pending" | ||
| assert step.progress == 0.0 |
| ] | ||
| ) | ||
|
|
||
| assert context.overall_progress == 0.5 |
There was a problem hiding this comment.
Since this is the only assert in the test, use math.isclose() instead of equality.
| title="Test" | ||
| ) | ||
|
|
||
| assert context.overall_progress == 0.0 |
There was a problem hiding this comment.
Since this is the only assert in the test, use math.isclose() instead of equality.
| progress.stop() | ||
|
|
||
| # Should complete without error | ||
| assert True |
There was a problem hiding this comment.
Replace this assert which is ineffective is really testing anything with a more stronger assert.
| time.sleep(0.2) | ||
|
|
||
| # Should complete without error | ||
| assert True |
There was a problem hiding this comment.
Replace this assert which is ineffective is really testing anything with a more stronger assert.
|
|
||
| assert tracker.steps[0].status == "completed" | ||
| assert tracker.steps[0].end_time is not None | ||
| assert tracker.steps[0].progress == 1.0 |
There was a problem hiding this comment.
use math.isclose() instead of equality.
| def test_operation_exception(self, indicator): | ||
| """Test operation that raises exception.""" | ||
| with pytest.raises(ValueError): | ||
| with indicator.operation("Test") as op: |
There was a problem hiding this comment.
Use the variable "op" in a meaningful way instead of removing it as suggested.
shanko
left a comment
shanko
left a comment
There was a problem hiding this comment.
I have commented on the changes that are recommended by the code quality bots. Most of them are "SGTM" (for Sounds Good To Me). Which means, those changes should be made in the code BEFORE merging this PR. A few of my comments have an explicit suggestion on what else needs to be done. Please do these changes and resubmit for review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
tests/test_progress_indicators.py (7)
9-12: Remove unused imports.These imports have been flagged as unused and acknowledged for removal.
127-127: Usemath.isclose()for floating-point comparison.As previously noted, use
math.isclose()instead of direct equality for floating-point assertions.
136-136: Usemath.isclose()for floating-point comparison.As previously noted, use
math.isclose()instead of direct equality for floating-point assertions.
159-161: Strengthen test assertion.The current assertion
assert Truedoesn't validate any behavior. As previously noted, replace with a meaningful assertion.Consider asserting that the progress completed without error or checking output:
progress.stop() - # Should complete without error - assert True + captured = capsys.readouterr() + # Verify spinner animation ran and stopped cleanly + assert "✓" in captured.out or "Updated" in captured.out or "Initial" in captured.out
218-220: Strengthen test assertion.The current assertion
assert Truedoesn't validate any behavior. As previously noted, replace with a meaningful assertion.Consider capturing output to verify the spinner ran:
with indicator.spinner("Loading..."): time.sleep(0.2) - # Should complete without error - assert True + captured = capsys.readouterr() + # Verify spinner completed with success marker + assert "✓" in captured.out or "Loading" in captured.out
354-356: Usemath.isclose()for floating-point comparison.As previously noted, use
math.isclose()instead of direct equality for floating-point assertions.+import math + # In test_complete_step: - assert tracker.steps[0].progress == 1.0 + assert math.isclose(tracker.steps[0].progress, 1.0)
474-478: Use theopvariable meaningfully.As previously requested, the
opvariable should be used in a meaningful way rather than being ignored.def test_operation_exception(self, indicator): """Test operation that raises exception.""" with pytest.raises(ValueError): with indicator.operation("Test") as op: + op.update("About to fail...") raise ValueError("Test error")cortex/progress_indicators.py (6)
13-13: Remove unused importCallable.As previously flagged,
Callableis imported but not used.
27-27: Remove unused importTaskProgressColumn.As previously flagged,
TaskProgressColumnis imported but not used.
34-34: Remove unused importLive.As previously flagged,
Livefromrich.liveis imported but not used.
35-35: Remove unused importPanel.As previously flagged,
Panelfromrich.panelis imported but not used.
37-37: Remove unused importText.As previously flagged,
Textfromrich.textis imported but not used.
700-700: Replace unused loop variablepkgwith_.As previously flagged, the loop variable is not used.
- for pkg in progress.progress_bar(packages, "Installing packages"): + for _ in progress.progress_bar(packages, "Installing packages"):
🧹 Nitpick comments (1)
docs/PROGRESS_INDICATORS.md (1)
335-349: Add language specifier to fenced code block.The architecture diagram code block should have a language specifier for proper rendering. Use
textorplaintextfor ASCII art diagrams.-``` +```text ┌─────────────────────────────────────────────────┐
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/progress_indicators.py(1 hunks)docs/PROGRESS_INDICATORS.md(1 hunks)tests/test_progress_indicators.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_progress_indicators.py (1)
cortex/progress_indicators.py (44)
ProgressIndicator(156-367)OperationType(46-58)OperationStep(62-78)OperationContext(82-105)FallbackProgress(109-153)DownloadTracker(486-543)MultiStepTracker(546-642)get_progress_indicator(649-654)spinner(240-257)spinner(658-660)operation(196-237)operation(663-665)progress_bar(259-298)progress_bar(668-670)duration_seconds(73-78)total_steps(94-95)completed_steps(98-99)overall_progress(102-105)start(119-124)stop(139-145)update(135-137)update(387-390)update(435-437)update(470-472)update(481-483)update(514-524)fail(147-153)fail(406-411)fail(448-451)fail(539-543)complete(399-404)complete(443-446)complete(526-537)print_success(341-346)print_error(348-353)print_warning(355-360)print_info(362-367)start_step(592-600)complete_step(602-611)fail_step(613-621)skip_step(623-629)finish(631-642)multi_step(316-339)download_progress(300-314)
🪛 LanguageTool
docs/PROGRESS_INDICATORS.md
[grammar] ~366-~366: Ensure spelling is correct
Context: ...Performance - Spinner updates: 10 FPS (100ms interval) - Progress bar: Updates on ea...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/PROGRESS_INDICATORS.md
335-335: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (18)
docs/PROGRESS_INDICATORS.md (1)
1-414: Documentation is comprehensive and well-structured.The documentation covers all aspects of the Progress Indicators module including API reference, usage examples, integration patterns, and troubleshooting. It aligns well with the implementation in
cortex/progress_indicators.py.tests/test_progress_indicators.py (7)
30-69: LGTM! OperationStep tests are thorough.Tests cover default values, duration calculations for not-started, running, and completed states. The timing assertions appropriately use ranges to account for execution variance.
71-137: LGTM! OperationContext tests provide good coverage.Tests validate default values, step counting, completion tracking, and progress calculation including the edge case of empty steps.
271-313: LGTM! DownloadTracker tests cover the essential lifecycle.Tests validate initialization, progressive updates, completion, and failure scenarios.
315-410: LGTM! MultiStepTracker tests are comprehensive.Tests cover initialization, step lifecycle (start, complete, fail, skip), finishing with various states, and out-of-bounds handling.
505-542: LGTM! Rich integration tests are properly guarded.The
@pytest.mark.skipifdecorator correctly skips these tests when Rich is unavailable, ensuring CI doesn't fail in minimal environments.
544-588: LGTM! Integration tests validate end-to-end flows.The tests simulate realistic installation and download workflows, verifying the components work together correctly.
232-236: No division by zero issue inprogress_barwith empty list.The fallback branch in
progress_barsafely handles empty lists. Whenitemsis empty, the for loop at line 292 (for i, item in enumerate(items):) produces zero iterations, so the division operation at line 293 (pct = (i + 1) / total * 100) is never executed. The test correctly verifies this behavior.cortex/progress_indicators.py (10)
46-59: LGTM! OperationType enum is well-defined.The enum covers all relevant operation types for the CLI tool with appropriate string values.
61-79: LGTM! OperationStep dataclass is well-designed.The
duration_secondsproperty elegantly handles both running and completed states.
81-106: LGTM! OperationContext dataclass provides useful computed properties.The progress calculation and step counting properties are correctly implemented with proper handling for empty steps.
108-154: LGTM! FallbackProgress provides a clean fallback implementation.The threaded spinner animation with proper cleanup on stop/fail is well implemented. The daemon thread ensures it won't block program exit.
156-194: LGTM! ProgressIndicator initialization is clean.The automatic fallback when Rich is unavailable and the comprehensive icon/color mappings are well thought out.
195-238: LGTM! Operation context manager handles lifecycle correctly.The exception handling properly marks operations as failed and the finally block ensures cleanup regardless of outcome.
370-424: LGTM! RichOperationHandle provides clean lifecycle management.The
__exit__method properly handles exceptions and auto-completion, and the status spinner is correctly started/stopped.
426-462: LGTM! FallbackOperationHandle mirrors Rich functionality.The fallback handle provides the same interface and behavior as the Rich version.
546-643: LGTM! MultiStepTracker is well-implemented.The step lifecycle management, bounds checking, and finish summary logic are all correct. The Rich rendering with appropriate icons for each state provides good visual feedback.
645-671: LGTM! Global convenience functions provide a clean API.The singleton pattern for
get_progress_indicatorand the wrapper functions (spinner,operation,progress_bar) provide a convenient module-level API.
| else: | ||
| for i, item in enumerate(items): | ||
| pct = (i + 1) / total * 100 | ||
| bar = "█" * int(pct / 5) + "░" * (20 - int(pct / 5)) | ||
| sys.stdout.write(f"\r{description}: [{bar}] {i+1}/{total}") | ||
| sys.stdout.flush() | ||
| yield item | ||
| print() # Newline after completion |
There was a problem hiding this comment.
Division by zero when iterating over empty list in fallback mode.
When items is an empty list, total will be 0, causing a ZeroDivisionError at line 293. The Rich branch handles this gracefully, but the fallback path does not.
else:
+ if total == 0:
+ return
for i, item in enumerate(items):
pct = (i + 1) / total * 100
bar = "█" * int(pct / 5) + "░" * (20 - int(pct / 5))
sys.stdout.write(f"\r{description}: [{bar}] {i+1}/{total}")
sys.stdout.flush()
yield item
print() # Newline after completion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| for i, item in enumerate(items): | |
| pct = (i + 1) / total * 100 | |
| bar = "█" * int(pct / 5) + "░" * (20 - int(pct / 5)) | |
| sys.stdout.write(f"\r{description}: [{bar}] {i+1}/{total}") | |
| sys.stdout.flush() | |
| yield item | |
| print() # Newline after completion | |
| else: | |
| if total == 0: | |
| return | |
| for i, item in enumerate(items): | |
| pct = (i + 1) / total * 100 | |
| bar = "█" * int(pct / 5) + "░" * (20 - int(pct / 5)) | |
| sys.stdout.write(f"\r{description}: [{bar}] {i+1}/{total}") | |
| sys.stdout.flush() | |
| yield item | |
| print() # Newline after completion |
🤖 Prompt for AI Agents
In cortex/progress_indicators.py around lines 291 to 298, the fallback progress
bar divides by total without checking for zero which raises ZeroDivisionError
when items is empty; add a guard before the loop: if total == 0 then print a
completed/empty progress line (e.g. description with an empty 20-char bar and
"0/0") and return/exit the generator early so no division occurs, otherwise
proceed with the existing loop logic.
| else: | ||
| pct = self.downloaded / self.total_bytes * 100 | ||
| speed = self.downloaded / (time.time() - self.start_time) / 1024 | ||
| sys.stdout.write(f"\r{self.description}: {pct:.1f}% ({speed:.1f} KB/s)") | ||
| sys.stdout.flush() |
There was a problem hiding this comment.
Potential division by zero in download progress calculation.
If total_bytes is 0 or if update is called immediately after initialization (when time.time() - self.start_time is nearly 0), this could cause a ZeroDivisionError.
def update(self, bytes_received: int):
"""Update with bytes received."""
self.downloaded += bytes_received
if self.indicator.use_rich and self._progress:
self._progress.update(self._task, completed=self.downloaded)
else:
- pct = self.downloaded / self.total_bytes * 100
- speed = self.downloaded / (time.time() - self.start_time) / 1024
+ elapsed = time.time() - self.start_time
+ pct = (self.downloaded / self.total_bytes * 100) if self.total_bytes > 0 else 0
+ speed = (self.downloaded / elapsed / 1024) if elapsed > 0 else 0
sys.stdout.write(f"\r{self.description}: {pct:.1f}% ({speed:.1f} KB/s)")
sys.stdout.flush()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| pct = self.downloaded / self.total_bytes * 100 | |
| speed = self.downloaded / (time.time() - self.start_time) / 1024 | |
| sys.stdout.write(f"\r{self.description}: {pct:.1f}% ({speed:.1f} KB/s)") | |
| sys.stdout.flush() | |
| else: | |
| elapsed = time.time() - self.start_time | |
| pct = (self.downloaded / self.total_bytes * 100) if self.total_bytes > 0 else 0 | |
| speed = (self.downloaded / elapsed / 1024) if elapsed > 0 else 0 | |
| sys.stdout.write(f"\r{self.description}: {pct:.1f}% ({speed:.1f} KB/s)") | |
| sys.stdout.flush() |
| duration = time.time() - self.start_time | ||
| speed = self.total_bytes / duration / 1024 / 1024 |
There was a problem hiding this comment.
Potential division by zero in download completion.
If complete() is called immediately after initialization, duration could be 0 or very close to 0, causing a ZeroDivisionError when calculating speed.
duration = time.time() - self.start_time
- speed = self.total_bytes / duration / 1024 / 1024
+ speed = (self.total_bytes / duration / 1024 / 1024) if duration > 0 else 0
self.indicator.print_success(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| duration = time.time() - self.start_time | |
| speed = self.total_bytes / duration / 1024 / 1024 | |
| duration = time.time() - self.start_time | |
| speed = (self.total_bytes / duration / 1024 / 1024) if duration > 0 else 0 |
🤖 Prompt for AI Agents
In cortex/progress_indicators.py around lines 533 to 534, calculating speed as
self.total_bytes / duration / 1024 / 1024 can raise a ZeroDivisionError if
duration is 0 or extremely small; guard against this by treating non-positive or
near-zero duration as a small epsilon (or directly setting speed to 0) before
dividing, e.g., compute duration = max(time.time() - self.start_time, 1e-9) or
branch to set speed = 0 when duration <= 0 so the division is safe and speed
remains sensible.
|
Implements #259 with complete tests and docs. Closes #259
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.