🎨 Palette: Fix syntax error & improve summary table UX#417
🎨 Palette: Fix syntax error & improve summary table UX#417
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 robustness and user experience of the application by addressing critical validation issues and improving the presentation of summary information. The changes ensure that data validation functions operate correctly and consistently, preventing runtime errors. Furthermore, the refactoring of the summary table generation and the introduction of human-readable duration formatting make the output much clearer and easier to interpret for users, especially for long-running processes. Highlights
Changelog
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
The pull request addresses critical syntax and logic errors, refactors the summary table for better maintainability, and introduces human-readable duration formatting. The changes significantly improve the robustness and user experience of the script. The validate_folder_data function now correctly handles nested rule groups and returns True on success, fixing a critical bug. The print_summary_table function has been refactored to use a new format_duration helper and a more modular approach for rendering the table, reducing code duplication. The new format_duration function provides clear, human-readable output for time durations. Overall, the changes are well-implemented and enhance the code quality.
I am having trouble creating individual review comments. Click here to see my feedback.
main.py (1139-1146)
This section contains a critical syntax error and logic error. The for loop and if statement are not correctly indented, leading to a SyntaxError. Additionally, the original code would return False if any rule was not a dictionary, but it should return True if all validations pass. The fix correctly indents the code and ensures the function returns True at the end if no issues are found.
# Ensure each rule within the group is an object (dict),
# because later code treats each rule as a mapping (e.g., rule.get(...)).
for j, rule in enumerate(rg.get("rules", [])):
if not isinstance(rule, dict):
log.error(
f"Invalid data from {sanitize_for_log(url)}: rule_groups[{i}].rules[{j}] must be an object."
)
return False
return True
main.py (318-328)
The format_duration function is a great addition. It significantly improves the readability of duration times in the summary table, especially for longer processes. This enhances the user experience by providing more intuitive feedback.
main.py (2328-2331)
Refactoring the separator logic into a make_sep helper function improves code readability and reduces duplication within the print_summary_table function. This is a good step towards more maintainable code.
def make_sep(left, mid, right, horiz=Box.H):
parts = [horiz * (width + 2) for width in w]
return f"{left}{mid.join(parts)}{right}"
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| print(f"\n{line('┌', '─', '┐')}") | ||
| title = f"{'DRY RUN' if dry_run else 'SYNC'} SUMMARY" | ||
| print(f"{Colors.BOLD}│{Colors.CYAN if dry_run else Colors.HEADER}{title:^{sum(w) + 14}}{Colors.ENDC}{Colors.BOLD}│{Colors.ENDC}") | ||
| print(f"{line('├', '┬', '┤')}\n{row([f'{Colors.HEADER}Profile ID{Colors.ENDC}', f'{Colors.HEADER}Folders{Colors.ENDC}', f'{Colors.HEADER}Rules{Colors.ENDC}', f'{Colors.HEADER}Duration{Colors.ENDC}', f'{Colors.HEADER}Status{Colors.ENDC}'])}") |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (120/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (141/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (125/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (111/100) Warning
|
|
||
| print(f"\n{line('┌', '─', '┐')}") | ||
| title = f"{'DRY RUN' if dry_run else 'SYNC'} SUMMARY" | ||
| print(f"{Colors.BOLD}│{Colors.CYAN if dry_run else Colors.HEADER}{title:^{sum(w) + 14}}{Colors.ENDC}{Colors.BOLD}│{Colors.ENDC}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| print(f"\n{line('┌', '─', '┐')}") | ||
| title = f"{'DRY RUN' if dry_run else 'SYNC'} SUMMARY" | ||
| print(f"{Colors.BOLD}│{Colors.CYAN if dry_run else Colors.HEADER}{title:^{sum(w) + 14}}{Colors.ENDC}{Colors.BOLD}│{Colors.ENDC}") | ||
| print(f"{line('├', '┬', '┤')}\n{row([f'{Colors.HEADER}Profile ID{Colors.ENDC}', f'{Colors.HEADER}Folders{Colors.ENDC}', f'{Colors.HEADER}Rules{Colors.ENDC}', f'{Colors.HEADER}Duration{Colors.ENDC}', f'{Colors.HEADER}Status{Colors.ENDC}'])}") |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (120/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (141/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (125/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (111/100) Warning
There was a problem hiding this comment.
Pull request overview
This PR aims to fix validate_folder_data correctness/runtime issues and improve the summary table UX by removing duplicated table-rendering logic and displaying human-readable durations.
Changes:
- Added
format_duration()to display durations asXm Ys/Xh Ym Zsinstead of seconds-only. - Updated
print_summary_table()to use the new duration formatter and refactored the Unicode table rendering to reduce duplication. - Adjusted
validate_folder_data()rule-group rule validation and ensured the function returnsTrueon success.
| d_str = format_duration(r['duration']) | ||
| print(f"{r['profile']:<{w[0]}} | {r['folders']:>{w[1]}} | {r['rules']:>{w[2]},} | {d_str:>{w[3]}} | {r['status_label']:<{w[4]}}") | ||
|
|
There was a problem hiding this comment.
format_duration() can produce strings longer than the fixed duration column width (w[3] is set to 10), which will break alignment/wrapping when printing rows. Consider deriving the duration column width from the maximum of len(format_duration(r['duration'])) across rows (and the total row) and at least len('Duration').
| f"Invalid data from {sanitize_for_log(url)} : rule_groups[fil].rules must be a list." | ||
| ) | ||
| return False |
There was a problem hiding this comment.
In the rule_groups validation, the logging/return block around this error string is still syntactically invalid (log. error and mis-indentation) and the message itself contains a placeholder (rule_groups[fil]...). This will prevent main.py from importing. Fix the block to use a valid log.error(...) call (no space after log.), correct indentation, and reference the actual index (e.g., rule_groups[{i}].rules must be a list) before returning False.
| def format_duration(seconds: float) -> str: | ||
| """Formats a duration in seconds into a human-readable string.""" | ||
| if seconds < 60: | ||
| return f"{seconds:.1f}s" | ||
|
|
||
| _print_separator("ml", "mm", "mr") | ||
| minutes, seconds = divmod(seconds, 60) | ||
| if minutes < 60: | ||
| return f"{int(minutes)}m {seconds:.1f}s" | ||
|
|
||
| # Total Row | ||
| total = len(results) | ||
| all_success = success_count == total | ||
|
|
||
| if dry_run: | ||
| total_status_text = "✅ Ready" if all_success else "❌ Errors" | ||
| else: | ||
| total_status_text = "✅ All Good" if all_success else "❌ Errors" | ||
|
|
||
| p_val = f"{'TOTAL':<{col_widths['profile']}}" | ||
| if USE_COLORS: | ||
| p_val = f"{Colors.BOLD}{p_val}{Colors.ENDC}" | ||
|
|
||
| f_val = f"{total_folders:>{col_widths['folders']}}" | ||
| r_val = f"{total_rules:>{col_widths['rules']},}" | ||
| d_val = f"{total_duration:>{col_widths['duration']-1}.1f}s" | ||
|
|
||
| s_val_raw = f"{total_status_text:<{col_widths['status']}}" | ||
| if USE_COLORS: | ||
| status_color = Colors.GREEN if all_success else Colors.FAIL | ||
| s_val = f"{status_color}{s_val_raw}{Colors.ENDC}" | ||
| else: | ||
| s_val = s_val_raw | ||
|
|
||
| print( | ||
| f"{chars['v']} {p_val} " | ||
| f"{chars['v']} {f_val} " | ||
| f"{chars['v']} {r_val} " | ||
| f"{chars['v']} {d_val} " | ||
| f"{chars['v']} {s_val} {chars['v']}" | ||
| ) | ||
|
|
||
| _print_separator("bl", "bm", "br") | ||
| hours, minutes = divmod(minutes, 60) | ||
| return f"{int(hours)}h {int(minutes)}m {seconds:.1f}s" |
There was a problem hiding this comment.
format_duration() introduces new formatting logic (seconds/minutes/hours) that is currently untested. Since this is deterministic and used in user-facing output, add small unit tests covering boundaries like 59.9s, 60.0s, 3599.9s, 3600.0s, and multi-digit hours to prevent regressions.
Understood. Acknowledging that this work is now obsolete as it has been superseded by #425 and #433. Stopping work on this task. |
💡 What:
SyntaxErrorinvalidate_folder_data.validate_folder_datareturnedNoneinstead ofTrue.print_summary_tableto eliminate duplicate/dead code.1m 30s).🎯 Why:
♿ Accessibility:
PR created automatically by Jules for task 755252452989555951 started by @abhimehro