Conversation
- Added `print_dry_run_plan` to display a user-friendly summary of changes during a dry run. - Updated `sync_profile` to call `print_dry_run_plan`. - Removed a duplicate definition of `render_progress_bar` (kept the one with docstring). - Added unit tests for the new output function. 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
|
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 tool by transforming the Highlights
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}
| # print() typically calls write(string) and write('\n') | ||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output | ||
| # ANSI codes should be present (main.Colors.HEADER starts with \033[95m) | ||
| assert "\033[" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output | ||
| # No ANSI codes | ||
| assert "\033[" not in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| main.print_dry_run_plan(plan) | ||
|
|
||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
| assert "(No folders found to sync)" in combined_output |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
![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 dry run output by providing a detailed plan of what will be created. It also includes a cleanup by removing a duplicate function definition. The changes are well-implemented and include new tests for the dry run output functionality. I've suggested a couple of refactorings to simplify the new printing function and its tests, which will improve code readability and maintainability.
main.py
Outdated
| profile = plan_entry["profile"] | ||
| folders = plan_entry["folders"] | ||
|
|
||
| if USE_COLORS: | ||
| print( | ||
| f"\n{Colors.HEADER}📝 Dry Run Plan for Profile: {Colors.BOLD}{profile}{Colors.ENDC}" | ||
| ) | ||
| print( | ||
| f"{Colors.CYAN} The following folders will be created/replaced:{Colors.ENDC}" | ||
| ) | ||
| else: | ||
| print(f"\n📝 Dry Run Plan for Profile: {profile}") | ||
| print(" The following folders will be created/replaced:") | ||
|
|
||
| if not folders: | ||
| print( | ||
| f" {Colors.WARNING}(No folders found to sync){Colors.ENDC}" | ||
| if USE_COLORS | ||
| else " (No folders found to sync)" | ||
| ) | ||
|
|
||
| for folder in folders: | ||
| name = folder["name"] | ||
| rules = folder["rules"] | ||
| if USE_COLORS: | ||
| print(f" • {Colors.BOLD}{name}{Colors.ENDC}: {rules} rules") | ||
| else: | ||
| print(f" - {name}: {rules} rules") | ||
| print("") |
There was a problem hiding this comment.
The print_dry_run_plan function can be simplified by fully leveraging the Colors class design. Since the class attributes are empty strings when colors are disabled, most of the explicit if USE_COLORS: checks are redundant. This refactoring reduces code duplication and improves readability. The conditional logic is only needed for the bullet point character, which is a deliberate UX choice.
profile = plan_entry["profile"]
folders = plan_entry["folders"]
print(f"\n{Colors.HEADER}📝 Dry Run Plan for Profile: {Colors.BOLD}{profile}{Colors.ENDC}")
print(f"{Colors.CYAN} The following folders will be created/replaced:{Colors.ENDC}")
if not folders:
print(f" {Colors.WARNING}(No folders found to sync){Colors.ENDC}")
else:
bullet = "•" if USE_COLORS else "-"
for folder in folders:
name = folder["name"]
rules = folder["rules"]
print(f" {bullet} {Colors.BOLD}{name}{Colors.ENDC}: {rules} rules")
print("")| monkeypatch.setattr(main, "USE_COLORS", True) | ||
|
|
||
| # Since Colors class is evaluated at import time, we need to manually set color codes | ||
| # if the module was imported in a non-TTY environment | ||
| for attr, code in { | ||
| "HEADER": "\033[95m", | ||
| "BLUE": "\033[94m", | ||
| "CYAN": "\033[96m", | ||
| "GREEN": "\033[92m", | ||
| "WARNING": "\033[93m", | ||
| "FAIL": "\033[91m", | ||
| "ENDC": "\033[0m", | ||
| "BOLD": "\033[1m", | ||
| "UNDERLINE": "\033[4m", | ||
| }.items(): | ||
| monkeypatch.setattr(main.Colors, attr, code) |
There was a problem hiding this comment.
The current method of enabling colors for this test is a bit brittle. By manually setting each color code, the test can become outdated if the Colors class in main.py is updated. A more robust approach is to simulate a TTY environment and then reload the main module. This ensures that USE_COLORS is set correctly and the Colors class is initialized with the right values, just as it would be in a real TTY environment. You will need to add import importlib to the file's imports.
# Force a TTY-like environment to test color output correctly
monkeypatch.setattr(sys.stdout, "isatty", lambda: True)
monkeypatch.setattr(sys.stderr, "isatty", lambda: True)
monkeypatch.delenv("NO_COLOR", raising=False)
# Reload the main module to re-evaluate USE_COLORS and the Colors class
# based on the mocked environment.
import importlib
importlib.reload(main)
| return folder_success | ||
|
|
||
|
|
||
| def print_dry_run_plan(plan_entry: Dict[str, Any]) -> None: |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning
| @@ -0,0 +1,89 @@ | |||
| import sys | |||
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing module docstring Warning test
| # print() typically calls write(string) and write('\n') | ||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output | ||
| # ANSI codes should be present (main.Colors.HEADER starts with \033[95m) | ||
| assert "\033[" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output | ||
| # No ANSI codes | ||
| assert "\033[" not in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| main.print_dry_run_plan(plan) | ||
|
|
||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
| assert "(No folders found to sync)" in combined_output |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| return folder_success | ||
|
|
||
|
|
||
| def print_dry_run_plan(plan_entry: Dict[str, Any]) -> None: |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing function docstring Warning
| @@ -0,0 +1,89 @@ | |||
| import sys | |||
Check warning
Code scanning / Pylint (reported by Codacy)
Missing module docstring Warning test
| # Since Colors class is evaluated at import time, we need to manually set color codes | ||
| # if the module was imported in a non-TTY environment | ||
| for attr, code in { | ||
| "HEADER": "\033[95m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| # if the module was imported in a non-TTY environment | ||
| for attr, code in { | ||
| "HEADER": "\033[95m", | ||
| "BLUE": "\033[94m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| for attr, code in { | ||
| "HEADER": "\033[95m", | ||
| "BLUE": "\033[94m", | ||
| "CYAN": "\033[96m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| "BLUE": "\033[94m", | ||
| "CYAN": "\033[96m", | ||
| "GREEN": "\033[92m", | ||
| "WARNING": "\033[93m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| "CYAN": "\033[96m", | ||
| "GREEN": "\033[92m", | ||
| "WARNING": "\033[93m", | ||
| "FAIL": "\033[91m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| "GREEN": "\033[92m", | ||
| "WARNING": "\033[93m", | ||
| "FAIL": "\033[91m", | ||
| "ENDC": "\033[0m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| "WARNING": "\033[93m", | ||
| "FAIL": "\033[91m", | ||
| "ENDC": "\033[0m", | ||
| "BOLD": "\033[1m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
| "FAIL": "\033[91m", | ||
| "ENDC": "\033[0m", | ||
| "BOLD": "\033[1m", | ||
| "UNDERLINE": "\033[4m", |
Check warning
Code scanning / Pylint (reported by Codacy)
Wrong hanging indentation before block (add 4 spaces). Warning test
There was a problem hiding this comment.
Pull request overview
This PR enhances the user experience of the CLI tool's dry-run mode by adding detailed output that shows which folders would be created and how many rules each contains. It also removes a duplicate render_progress_bar function definition.
Changes:
- Added
print_dry_run_plan()function to display a formatted plan when--dry-runis used - Integrated the new function into the dry-run workflow to provide detailed output before the completion message
- Removed duplicate
render_progress_bar()function definition that had slightly different parameters
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| main.py | Added print_dry_run_plan() function with color-aware formatting and removed duplicate render_progress_bar() definition |
| tests/test_palette_ux.py | Added comprehensive test coverage for the new function covering colored output, non-colored output, and empty folder scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_print_dry_run_plan_empty_folders(monkeypatch): | ||
| """Verify output when no folders are present.""" | ||
| monkeypatch.setattr(main, "USE_COLORS", False) | ||
| mock_stdout = MagicMock() | ||
| monkeypatch.setattr(sys, "stdout", mock_stdout) | ||
|
|
||
| plan = { | ||
| "profile": "test_profile", | ||
| "folders": [] | ||
| } | ||
|
|
||
| main.print_dry_run_plan(plan) | ||
|
|
||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
| assert "(No folders found to sync)" in combined_output |
There was a problem hiding this comment.
The test should also set the Colors class attributes to empty strings when USE_COLORS is False. If the module was imported in a TTY environment, the Colors attributes would still contain ANSI codes even after setting USE_COLORS to False, which could cause the test to fail.
The same pattern from test_print_dry_run_plan_with_colors should be applied here, setting all Colors attributes to empty strings:
- HEADER, BLUE, CYAN, GREEN, WARNING, FAIL, ENDC, BOLD, UNDERLINE should all be set to ""
| def test_print_dry_run_plan_no_colors(monkeypatch): | ||
| """Verify print_dry_run_plan output without colors.""" | ||
| monkeypatch.setattr(main, "USE_COLORS", False) | ||
|
|
||
| mock_stdout = MagicMock() | ||
| monkeypatch.setattr(sys, "stdout", mock_stdout) | ||
|
|
||
| plan = { | ||
| "profile": "test_profile", | ||
| "folders": [ | ||
| {"name": "Test Folder 1", "rules": 10}, | ||
| ] | ||
| } | ||
|
|
||
| main.print_dry_run_plan(plan) | ||
|
|
||
| combined_output = "".join([str(args[0]) for args, _ in mock_stdout.write.call_args_list]) | ||
|
|
||
| assert "📝 Dry Run Plan for Profile:" in combined_output | ||
| assert "test_profile" in combined_output | ||
| assert "Test Folder 1" in combined_output | ||
| assert "10 rules" in combined_output | ||
| # No ANSI codes | ||
| assert "\033[" not in combined_output |
There was a problem hiding this comment.
The test should also set the Colors class attributes to empty strings when USE_COLORS is False, similar to how test_print_dry_run_plan_with_colors manually sets color codes. If the module was imported in a TTY environment, the Colors attributes would still contain ANSI codes even after setting USE_COLORS to False, which could cause the test to fail.
The same pattern from test_print_dry_run_plan_with_colors should be applied here, setting all Colors attributes to empty strings:
- HEADER, BLUE, CYAN, GREEN, WARNING, FAIL, ENDC, BOLD, UNDERLINE should all be set to ""
- Added `print_dry_run_plan` to display a user-friendly summary of changes during a dry run. - Updated `sync_profile` to call `print_dry_run_plan`. - Used `sanitize_for_log` on printed user inputs to satisfy CodeQL security checks. - Removed a duplicate definition of `render_progress_bar`. - Added unit tests for the new output function. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
- Added `print_dry_run_plan` to display a user-friendly summary of changes during a dry run. - Updated `sync_profile` to call `print_dry_run_plan`. - Used `sanitize_for_log` on printed user inputs to satisfy CodeQL security checks. - Removed a duplicate definition of `render_progress_bar`. - Added unit tests for the new output function. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
abhimehro
deleted the
palette-dry-run-plan-output-15215056721859509049
branch
…itization, add dry-run plan details Incorporates the best changes from 36 Jules PRs, addressing review feedback: Bolt (Performance) - from PR #173: - Pre-compile PROFILE_ID_PATTERN and RULE_PATTERN at module level - Use compiled patterns in is_valid_profile_id_format, validate_profile_id, and is_valid_rule - Supersedes PRs: #140, #143, #152, #155, #158, #161, #167, #170, #173 Sentinel (Security) - from PR #172 with review feedback: - Enhance sanitize_for_log to redact Basic Auth credentials in URLs - Redact sensitive query parameters (token, key, secret, password, etc.) - Handle fragment separators (#) per Gemini Code Assist review - Use [^&#\s]* pattern per Copilot reviewer suggestion - Update docstring per reviewer suggestion - Supersedes PRs: #142, #145, #148, #151, #154, #157, #160, #169, #172 Palette (UX) - from PR #174 with lint fixes: - Add print_plan_details function for dry-run visibility - Fix duplicate render_progress_bar definition bug - Supersedes PRs: #139, #141, #144, #147, #150, #153, #156, #159, #162, #165, #168, #171, #174 Also: #146, #149, #164 (parallel folder deletion) and #166 (auto-fix .env perms) are independent features not consolidated here. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
This PR improves the UX of the CLI tool by printing a detailed plan when running with
--dry-run.Previously, it only said "Dry-run complete: no API calls were made". Now, it lists the folders that would be created and how many rules each contains.
Also cleaned up a duplicate function definition in
main.py.PR created automatically by Jules for task 15215056721859509049 started by @abhimehro