🎨 Palette: Human-readable durations & code cleanup#424
🎨 Palette: Human-readable durations & code cleanup#424
Conversation
…rror - Fix critical syntax error in `validate_folder_data` that prevented validation. - Refactor `print_summary_table` to be a single, robust source of truth. - Add `format_duration` helper to display times as `1m 30s` instead of raw seconds. - Remove duplicate inline table printing logic from `main()`. 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 enhances the command-line interface by making duration outputs more user-friendly, which significantly improves the readability of timing data. It also includes important code quality improvements by refactoring duplicated table rendering logic and fixing a critical syntax error in data validation, leading to a more robust and maintainable codebase. 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| **Learning:** Reconstructing command arguments manually for process restarts is brittle and breaks forward compatibility. | ||
| **Action:** When restarting a CLI tool with modified flags (e.g., removing `--dry-run`), filter `sys.argv` instead of rebuilding the argument list from parsed args. | ||
|
|
||
| ## 2025-05-20 - [Human Readable Durations] |
Check notice
Code scanning / Remark-lint (reported by Codacy)
Warn when references to undefined definitions are found. Note
| **Learning:** Reconstructing command arguments manually for process restarts is brittle and breaks forward compatibility. | ||
| **Action:** When restarting a CLI tool with modified flags (e.g., removing `--dry-run`), filter `sys.argv` instead of rebuilding the argument list from parsed args. | ||
|
|
||
| ## 2025-05-20 - [Human Readable Durations] |
Check notice
Code scanning / Remark-lint (reported by Codacy)
Warn when shortcut reference links are used. Note
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| m, s = divmod(seconds, 60) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "s" doesn't conform to snake_case naming style Warning
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| m, s = divmod(seconds, 60) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "m" doesn't conform to snake_case naming style Warning
| if m < 60: | ||
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "m" doesn't conform to snake_case naming style Warning
| if m < 60: | ||
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "h" doesn't conform to snake_case naming style Warning
| table_width = len(dummy_sep) | ||
|
|
||
| title_text = " DRY RUN SUMMARY " if dry_run else " SYNC SUMMARY " | ||
| title_color = Colors.CYAN if dry_run else Colors.HEADER |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (120/100) Warning
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| m, s = divmod(seconds, 60) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "s" doesn't conform to snake_case naming style Warning
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| m, s = divmod(seconds, 60) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "m" doesn't conform to snake_case naming style Warning
| if m < 60: | ||
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "h" doesn't conform to snake_case naming style Warning
| if m < 60: | ||
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Variable name "m" doesn't conform to snake_case naming style Warning
| table_width = len(dummy_sep) | ||
|
|
||
| title_text = " DRY RUN SUMMARY " if dry_run else " SYNC SUMMARY " | ||
| title_color = Colors.CYAN if dry_run else Colors.HEADER |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (120/100) Warning
| dummy_sep = make_col_separator(Box.TL, Box.T, Box.TR, Box.H) | ||
| table_width = len(dummy_sep) | ||
|
|
||
| title_text = " DRY RUN SUMMARY " if dry_run else " SYNC SUMMARY " |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI user experience by adding human-readable duration formatting and fixes a critical syntax error in validation code. The changes consolidate duplicate table-printing logic into a single function while enhancing readability of timing information.
Changes:
- Added
format_duration()helper function to convert seconds into human-readable format (e.g., "2m 5.5s" instead of "125.5s") - Fixed syntax errors in
validate_folder_data()where rule validation code had garbled characters and broken formatting - Removed duplicate
print_summary_table()function definition and consolidated table printing logic into a single reusable function
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| main.py | Added format_duration() utility function (lines 274-285), fixed syntax errors in rule validation block (lines 1004-1013), removed duplicate function definition (lines 329-480), refactored print_summary_table() to use new formatting (lines 2303-2404), simplified main() to call consolidated function (lines 2641-2642) |
| .Jules/palette.md | Documented learning about human-readable duration formatting for CLI tools (lines 13-15) |
| def format_duration(seconds: float) -> str: | ||
| """Format duration in a human-readable way (e.g. 1m 30s).""" | ||
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| m, s = divmod(seconds, 60) | ||
| if m < 60: | ||
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) | ||
| return f"{int(h)}h {int(m)}m {s:.0f}s" |
There was a problem hiding this comment.
The new format_duration() function lacks test coverage. Given the comprehensive test suite in this repository (test_main.py and tests/ directory), this utility function should have tests covering:
- Durations less than 60 seconds (e.g., 45.5s → "45.5s")
- Durations in minutes (e.g., 125.5s → "2m 5.5s")
- Durations with hours (e.g., 3661s → "1h 1m 1s")
- Edge cases like 0.0 seconds and very small values
This is especially important since the function is now used in user-facing summary tables where incorrect formatting would be immediately visible.
![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 introduces a helper function to format durations into a human-readable format and refactors the summary table printing logic to improve code quality and readability, significantly cleaning up duplicated code. However, a critical Denial of Service vulnerability was identified in the validate_folder_data function due to the use of an undefined variable fil when processing malformed JSON data; this should be fixed by using the correct loop index i. Additionally, there are a couple of suggestions to further improve the consistency and robustness of the new summary table.
| return f"{int(m)}m {s:.1f}s" | ||
|
|
||
| h, m = divmod(m, 60) | ||
| return f"{int(h)}h {int(m)}m {s:.0f}s" |
There was a problem hiding this comment.
The formatting of seconds is inconsistent across different duration ranges. For durations less than an hour, seconds are displayed with one decimal place (e.g., 59.9s), but for durations of an hour or more, they are rounded to the nearest whole number. This might be intentional, but for a more consistent user experience, consider using one decimal place for seconds in all cases.
| return f"{int(h)}h {int(m)}m {s:.0f}s" | |
| return f"{int(h)}h {int(m)}m {s:.1f}s" |
| w_folders = 10 | ||
| w_rules = 12 | ||
| w_duration = 10 |
There was a problem hiding this comment.
The column widths for "Folders", "Rules", and "Duration" are hardcoded. This can cause the table formatting to break if the values for these columns exceed the fixed width (e.g., a very long duration or a large number of rules). To make the table more robust, I suggest calculating these widths dynamically based on the content, similar to how w_profile is handled. Note that for this to work, you'll need to move the total_folders, total_rules, and total_duration calculations to before these width assignments.
| w_folders = 10 | |
| w_rules = 12 | |
| w_duration = 10 | |
| w_folders = max(10, len(f"{total_folders:,}"), *(len(f"{r['folders']:,}") for r in sync_results)) | |
| w_rules = max(12, len(f"{total_rules:,}"), *(len(f"{r['rules']:,}") for r in sync_results)) | |
| w_duration = max(10, len(format_duration(total_duration)), *(len(format_duration(r['duration'])) for r in sync_results)) |
Understood. Acknowledging that this work is now obsolete as it has been superseded. Stopping work on this task. |
abhimehro
deleted the
palette-human-readable-durations-7775783598022719315
branch
💡 What: Added human-readable duration formatting to the CLI summary table and fixed a syntax error.
🎯 Why: Long durations in seconds are hard to parse. The code also contained duplicate table printing logic and a broken validation block.
📸 Before/After: Durations like
125.5snow show as2m 5.5s.♿ Accessibility: Improved readability of timing data.
PR created automatically by Jules for task 7775783598022719315 started by @abhimehro