🎨 Palette: Improve CLI Feedback & Formatting#190
Conversation
Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
|
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
Summary of ChangesHello @abhimehro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the CLI by providing clearer feedback during long-running operations and improving the readability of duration displays. It replaces static, silent waits with dynamic countdown timers, reducing user anxiety about application responsiveness. Additionally, it refines how durations are presented in summary tables, making them easier to comprehend at a glance. These changes also include a fix for a test suite to ensure consistent behavior across different environments. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the command-line interface experience by providing better feedback during long-running operations. The introduction of format_duration makes time reporting more human-readable, and replacing silent sleep calls with an interactive countdown_timer is a great enhancement for user feedback. The fix in the tests to use monkeypatch for mocking globals is also a good move towards more robust testing. I have one suggestion in main.py to make the format_duration function slightly more concise and idiomatic. Overall, these are excellent changes.
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI user experience by making long waits visible (countdown/progress feedback) and by formatting sync durations more readably in the summary output, plus stabilizing a flaky test around USE_COLORS.
Changes:
- Added
format_duration()and used it in the summary table output. - Replaced
time.sleep(...)waits withcountdown_timer(...)in retry/polling paths. - Updated
tests/test_plan_details.pyto usemonkeypatchforUSE_COLORSinstead ofunittest.mock.patch.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
main.py |
Adds duration formatting and swaps silent waits for interactive countdowns in retry/polling + summary formatting changes. |
tests/test_plan_details.py |
Switches to monkeypatch for USE_COLORS to make tests less brittle. |
.python-version |
Normalizes the single-line file formatting (no semantic change). |
| wait_time = RETRY_DELAY * (2**attempt) | ||
| log.warning( | ||
| "Request failed (attempt %d/%d). Retrying in %ds...", | ||
| attempt + 1, | ||
| MAX_RETRIES, | ||
| wait_time, | ||
| ) | ||
| time.sleep(wait_time) | ||
| countdown_timer(int(wait_time), "Retrying") |
There was a problem hiding this comment.
PR description says silent sleeps were replaced during "API retries", but only verify_access_and_get_folders() uses countdown_timer() here. Other API calls still go through _retry_request() which continues to time.sleep(wait_time) (main.py:606-622), so users may still see silent backoff waits. Either update _retry_request() to use countdown_timer() (when interactive) or narrow the PR description/intent.
| def format_duration(seconds: float) -> str: | ||
| """Formats duration in a human-readable way (e.g., 2m 05s).""" | ||
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| minutes = int(seconds // 60) | ||
| rem_seconds = int(seconds % 60) | ||
| return f"{minutes}m {rem_seconds:02d}s" |
There was a problem hiding this comment.
format_duration() is new user-visible formatting logic but currently has no direct unit tests. Adding focused tests for boundary conditions (e.g., <60s, exactly 60s, rounding behavior) will prevent regressions in CLI output formatting.
| wait_time, | ||
| ) | ||
| time.sleep(wait_time) | ||
| countdown_timer(int(wait_time), "Retrying") |
There was a problem hiding this comment.
Now that this call uses countdown_timer(), note that the current implementation writes \r...{remaining}s... without clearing the line each tick. When remaining drops from 10→9 (or 100→99), terminals can show leftover characters from the previous, longer line. Consider updating countdown_timer() to clear the line on every update (e.g., prefix each frame with \r\033[K, similar to render_progress_bar).
main.py
Outdated
| """Formats duration in a human-readable way (e.g., 2m 05s).""" | ||
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| minutes = int(seconds // 60) | ||
| rem_seconds = int(seconds % 60) |
There was a problem hiding this comment.
format_duration() truncates seconds via int(seconds % 60) and formats sub-minute durations with seconds:.1f. This can produce surprising outputs around boundaries (e.g., 59.95s -> "60.0s" instead of "1m 00s") and under-report durations (125.9s -> "2m 05s"). Consider rounding to the nearest whole second first, then deriving minutes/seconds from that (and using a consistent Xm YYs format).
| """Formats duration in a human-readable way (e.g., 2m 05s).""" | |
| if seconds < 60: | |
| return f"{seconds:.1f}s" | |
| minutes = int(seconds // 60) | |
| rem_seconds = int(seconds % 60) | |
| """Formats duration in a human-readable way (e.g., 2m 05s). | |
| We first round to the nearest whole second to avoid surprising | |
| outputs around boundaries (e.g., 59.95s -> 1m 00s instead of | |
| 60.0s) and then derive minutes/seconds from that integer. | |
| """ | |
| # Normalize to an integer number of seconds via rounding, not truncation. | |
| total_seconds = int(round(seconds)) | |
| # For sub-minute durations, show whole seconds (e.g., "5s", "42s"). | |
| if total_seconds < 60: | |
| return f"{total_seconds:d}s" | |
| # For longer durations, show "Xm YYs" using the rounded total seconds. | |
| minutes, rem_seconds = divmod(total_seconds, 60) |
- No code changes made. - Responding to PR comment regarding supersession. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
main.py
Outdated
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| minutes, rem_seconds = divmod(int(seconds), 60) | ||
| return f'{minutes}m {rem_seconds:02d}s' | ||
|
|
||
|
|
There was a problem hiding this comment.
format_duration() can produce misleading output around the 60s boundary (e.g., seconds=59.95 formats as 60.0s because rounding happens after the < 60 check). Also, for >= 60 it truncates fractional seconds via int(seconds), which can underreport durations (especially the TOTAL where floats are summed). Consider rounding once up front (e.g., to whole seconds) before branching/divmod so 59.95 becomes 1m 00s and totals are consistent.
| if seconds < 60: | |
| return f"{seconds:.1f}s" | |
| minutes, rem_seconds = divmod(int(seconds), 60) | |
| return f'{minutes}m {rem_seconds:02d}s' | |
| # Round once to whole seconds so behavior is consistent around the 60s | |
| # boundary and we don't under-report longer durations due to truncation. | |
| total_seconds = int(round(seconds)) | |
| if total_seconds < 60: | |
| # For sub-minute durations, show whole seconds for clarity and to | |
| # match the rounded value used for longer durations. | |
| return f"{total_seconds}s" | |
| minutes, rem_seconds = divmod(total_seconds, 60) | |
| return f"{minutes}m {rem_seconds:02d}s" |
main.py
Outdated
| f"Folder '{sanitize_for_log(name)}' not found yet. Retrying in {wait_time}s..." | ||
| ) | ||
| time.sleep(wait_time) | ||
| # Local countdown with line clearing to avoid trailing characters when digits shrink (e.g., 10→9) |
There was a problem hiding this comment.
With the new use of countdown_timer() here, be aware countdown_timer() doesn’t clear the line on each tick (it only clears once at the end). When remaining seconds shrink (e.g., 10→9), trailing characters can be left on screen. Consider updating countdown_timer() to include \033[K (or fixed-width padding) per iteration so the countdown renders cleanly.
…wn timer UX, and add tests Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
…wn timer UX, and add tests Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
💡 What:
format_durationhelper to display durations inXm Ysformat (e.g.,2m 05s) in the summary table.countdown_timerduring API retries and folder creation polling.tests/test_plan_details.pyto correctly mockUSE_COLORS.🎯 Why:
125.5s) is harder to parse than2m 05s.📸 Before/After:
Duration: 125.5s| Silent wait for 60s.Duration: 2m 05s|⏳ Waiting for deletions to propagate: [████░░] 30s...♿ Accessibility:
PR created automatically by Jules for task 5962504483824375634 started by @abhimehro