🎨 Palette: Improve CLI output formatting#217
Conversation
Added thousands separators to rule counts and aligned folder names in `print_plan_details` for better readability. Also added comma formatting to other log messages. 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. |
|
😎 Merged manually by @abhimehro - details. |
|
👋 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 focuses on enhancing the user experience of the CLI by refining its output formatting. The changes introduce dynamic alignment for tabular data and apply standard comma separators to large numerical values, making the information presented in dry-run plans and various log messages significantly easier to read and comprehend. 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}
![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 improves the CLI output formatting by aligning columns in the plan details and adding comma separators for large numbers, which enhances readability. The changes are well-implemented. I've identified a minor bug and a performance issue in the print_plan_details function and provided a suggestion to refactor it for correctness and efficiency. The rest of the changes look good.
| # Calculate max width for alignment | ||
| max_name_len = max( | ||
| (len(sanitize_for_log(f.get("name", ""))) for f in folders), default=0 | ||
| ) | ||
| max_rules_len = max((len(f"{f.get('rules', 0):,}") for f in folders), default=0) | ||
|
|
||
| for folder in sorted(folders, key=lambda f: f.get("name", "")): | ||
| name = sanitize_for_log(folder.get("name", "Unknown")) | ||
| rules_count = folder.get("rules", 0) | ||
| formatted_rules = f"{rules_count:,}" | ||
|
|
||
| if USE_COLORS: | ||
| print(f" • {Colors.BOLD}{name}{Colors.ENDC}: {rules_count} rules") | ||
| print( | ||
| f" • {Colors.BOLD}{name:<{max_name_len}}{Colors.ENDC} : {formatted_rules:>{max_rules_len}} rules" | ||
| ) | ||
| else: | ||
| print(f" - {name}: {rules_count} rules") | ||
| print( | ||
| f" - {name:<{max_name_len}} : {formatted_rules:>{max_rules_len}} rules" | ||
| ) |
There was a problem hiding this comment.
This implementation has a subtle bug and a performance issue.
Bug: The max_name_len calculation uses f.get("name", ""), but the loop uses folder.get("name", "Unknown"). If a folder has no name, its length will be calculated based on an empty string, but it will be printed as "Unknown" (length 7), which will break the column alignment this PR aims to fix.
Performance: sanitize_for_log() and the rule count formatting are performed for each folder multiple times (once for max length calculation, and again in the printing loop).
I suggest refactoring to pre-process the folder data once. This fixes the bug, improves efficiency by avoiding redundant computations, and also makes the sorting behavior more intuitive by sorting based on the final displayed name.
# Pre-process folders to get display values and avoid re-computation
display_data = [
{
"name": sanitize_for_log(f.get("name", "Unknown")),
"formatted_rules": f"{f.get('rules', 0):,}",
}
for f in folders
]
# Calculate max width for alignment from pre-processed data
max_name_len = max((len(f["name"]) for f in display_data), default=0)
max_rules_len = max((len(f["formatted_rules"]) for f in display_data), default=0)
for folder in sorted(display_data, key=lambda f: f["name"]):
name = folder["name"]
formatted_rules = folder["formatted_rules"]
if USE_COLORS:
print(
f" • {Colors.BOLD}{name:<{max_name_len}}{Colors.ENDC} : {formatted_rules:>{max_rules_len}} rules"
)
else:
print(
f" - {name:<{max_name_len}} : {formatted_rules:>{max_rules_len}} rules"
)There was a problem hiding this comment.
Pull request overview
This PR improves the CLI output formatting by enhancing the visual presentation of the dry-run plan output and adding comma separators to large numbers throughout the logging system.
Changes:
- Enhanced
print_plan_detailsto dynamically calculate column widths for aligned folder names and rule counts - Added comma thousand-separators to rule counts and URL counts in logs for better readability
- Improved spacing around colons in the plan output (changed from
:to:)
| # Calculate max width for alignment | ||
| max_name_len = max( | ||
| (len(sanitize_for_log(f.get("name", ""))) for f in folders), default=0 | ||
| ) | ||
| max_rules_len = max((len(f"{f.get('rules', 0):,}") for f in folders), default=0) | ||
|
|
||
| for folder in sorted(folders, key=lambda f: f.get("name", "")): | ||
| name = sanitize_for_log(folder.get("name", "Unknown")) | ||
| rules_count = folder.get("rules", 0) | ||
| formatted_rules = f"{rules_count:,}" | ||
|
|
||
| if USE_COLORS: | ||
| print(f" • {Colors.BOLD}{name}{Colors.ENDC}: {rules_count} rules") | ||
| print( | ||
| f" • {Colors.BOLD}{name:<{max_name_len}}{Colors.ENDC} : {formatted_rules:>{max_rules_len}} rules" | ||
| ) | ||
| else: | ||
| print(f" - {name}: {rules_count} rules") | ||
| print( | ||
| f" - {name:<{max_name_len}} : {formatted_rules:>{max_rules_len}} rules" | ||
| ) |
There was a problem hiding this comment.
The output format changes in print_plan_details will break existing tests. The test assertions in tests/test_plan_details.py (lines 31-32) check for the old format without spaces around the colon (e.g., "Folder A: 10 rules") but the new format has spaces around the colon (e.g., "Folder A : 10 rules"). Additionally, the tests don't account for comma formatting of numbers. The tests need to be updated to match the new format.
- Split `get_validated_input` into `get_validated_input` (public) and `get_password` (sensitive). - Aligned folder names and rule counts in the dry-run plan output. - Added thousands separators to rule counts and other large numbers in logs. - Updated tests to cover new input handling logic. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
| m.get_password("Prompt: ", lambda x: True, "Error") | ||
|
|
||
| # Check exit code is 130 | ||
| assert e.value.code == 130 |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # Check friendly message | ||
| captured = capsys.readouterr() | ||
| assert "Input cancelled" in captured.out |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| @pytest.mark.parametrize("exception", [KeyboardInterrupt, EOFError]) | ||
| def test_get_validated_input_graceful_exit(monkeypatch, capsys, exception): | ||
| """Test graceful exit on user cancellation (Ctrl+C/Ctrl+D) for regular inputs.""" | ||
| m = reload_main_with_env(monkeypatch) |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # Check friendly cancellation message is displayed | ||
| captured = capsys.readouterr() | ||
| assert "Input cancelled" in captured.out |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
| # Case 12: get_password works with getpass | ||
| def test_get_password(monkeypatch): | ||
| m = reload_main_with_env(monkeypatch) | ||
|
|
||
| getpass_mock = MagicMock(return_value="secret") | ||
| monkeypatch.setattr("getpass.getpass", getpass_mock) | ||
|
|
||
| validator = lambda x: True | ||
|
|
||
| result = m.get_validated_input("Password: ", validator, "Error", is_password=True) | ||
| result = m.get_password("Password: ", validator, "Error") | ||
|
|
||
| assert result == "secret" | ||
| getpass_mock.assert_called_once() |
There was a problem hiding this comment.
The test test_get_password only covers the happy path where the validator returns True. Consider adding a test similar to test_get_validated_input_retry that tests the retry logic for get_password, including:
- Empty password input (should show "Value cannot be empty")
- Invalid password that fails validation (should show the error message)
- Valid password on subsequent retry
This would ensure both functions have equivalent test coverage.
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
abhimehro
deleted the
palette-improve-cli-output-formatting-43751772871382236
branch
Improved the CLI output by:
1,234) to rule counts and other large numbers in logs.print_plan_detailsto dynamically calculate column widths.PR created automatically by Jules for task 43751772871382236 started by @abhimehro