Conversation
- Added detailed plan output to `sync_profile` when running in dry-run mode. - It now lists the folders that would be created and their rule counts. - Removed a duplicate definition of `render_progress_bar`. - Improved UX by providing clear feedback on what the script will do. 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 improves the user experience of the dry-run functionality by introducing a comprehensive output that details the planned operations, including folder structures and rule counts. This enhancement empowers users to thoroughly review and validate their configurations without performing any actual modifications. Concurrently, the PR contributes to code quality by eliminating a redundant function definition. 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
|
![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 enhances the dry-run output and removes a duplicated function, which is a good cleanup. I've found an opportunity to further improve the dry-run output by making its behavior more consistent and reducing some code duplication in the process. My suggestion ensures the plan is always printed to standard output, which improves usability, especially for command-line redirection.
| if USE_COLORS: | ||
| print(f"\n{Colors.CYAN}🔍 Plan for profile {profile_id}:{Colors.ENDC}") | ||
| for folder in plan_entry["folders"]: | ||
| f_name = folder["name"] | ||
| f_rules = folder["rules"] | ||
| print(f" • {f_name} ({f_rules:,} rules)") | ||
| print("") | ||
| else: | ||
| log.info(f"Plan for profile {profile_id}:") | ||
| for folder in plan_entry["folders"]: | ||
| f_name = folder["name"] | ||
| f_rules = folder["rules"] | ||
| log.info(f" - {f_name} ({f_rules:,} rules)") |
There was a problem hiding this comment.
The dry-run plan output is user-facing information and should consistently be printed to standard output (stdout). Currently, the colored output goes to stdout via print(), but the non-colored output is sent to standard error (stderr) via log.info(). This is inconsistent and can be confusing for users who might want to redirect the plan to a file (e.g., ... > plan.txt).
By using print() for both cases, we can ensure consistent behavior and also simplify the code by removing the duplicated loop.
| if USE_COLORS: | |
| print(f"\n{Colors.CYAN}🔍 Plan for profile {profile_id}:{Colors.ENDC}") | |
| for folder in plan_entry["folders"]: | |
| f_name = folder["name"] | |
| f_rules = folder["rules"] | |
| print(f" • {f_name} ({f_rules:,} rules)") | |
| print("") | |
| else: | |
| log.info(f"Plan for profile {profile_id}:") | |
| for folder in plan_entry["folders"]: | |
| f_name = folder["name"] | |
| f_rules = folder["rules"] | |
| log.info(f" - {f_name} ({f_rules:,} rules)") | |
| if USE_COLORS: | |
| print(f"\n{Colors.CYAN}🔍 Plan for profile {profile_id}:{Colors.ENDC}") | |
| line_prefix = " • " | |
| else: | |
| print(f"Plan for profile {profile_id}:") | |
| line_prefix = " - " | |
| for folder in plan_entry["folders"]: | |
| f_name = folder["name"] | |
| f_rules = folder["rules"] | |
| print(f"{line_prefix}{f_name} ({f_rules:,} rules)") | |
| if USE_COLORS: | |
| print("") |
There was a problem hiding this comment.
Pull request overview
This PR enhances the user experience of the --dry-run mode by displaying a detailed plan of folders and rules that would be created, allowing users to verify configuration before execution. It also removes a duplicate render_progress_bar function definition.
Changes:
- Removed duplicate
render_progress_barfunction (lines 162-180) that had width=20 instead of width=15 - Added detailed dry-run output showing folder names and rule counts per profile (lines 1113-1126)
- Output respects USE_COLORS flag, providing colored output for TTY and plain log messages otherwise
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added detailed plan output to `sync_profile` when running in dry-run mode. - It now lists the folders that would be created and their rule counts. - Removed a duplicate definition of `render_progress_bar`. - Improved UX by providing clear feedback on what the script will do. - Sanitized `profile_id` and folder names in log output to address CodeQL security alert. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Added detailed plan output to `sync_profile` when running in dry-run mode. - It now lists the folders that would be created and their rule counts. - Removed a duplicate definition of `render_progress_bar`. - Improved UX by providing clear feedback on what the script will do. - Addressed CodeQL security alerts by ensuring `profile_id` and folder names are explicitly sanitized and assigned to safe variables before logging. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
| print(f" • {safe_name} ({rule_count:,} rules)") | ||
| print("") | ||
| else: | ||
| log.info(f"Plan for profile {safe_profile_id}:") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| log.info(f" - {safe_name} ({rule_count:,} rules)") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| print(f" • {safe_name} ({rule_count:,} rules)") | ||
| print("") | ||
| else: | ||
| log.info(f"Plan for profile {safe_profile_id}:") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| log.info(f" - {safe_name} ({rule_count:,} rules)") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for folder in plan_entry["folders"]: | ||
| # Sanitize folder name and ensure rule count is an integer | ||
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| print(f" • {safe_name} ({rule_count:,} rules)") | ||
| print("") | ||
| else: | ||
| log.info(f"Plan for profile {safe_profile_id}:") | ||
| for folder in plan_entry["folders"]: | ||
| # Sanitize folder name and ensure rule count is an integer | ||
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| log.info(f" - {safe_name} ({rule_count:,} rules)") | ||
|
|
There was a problem hiding this comment.
The code for printing folder information is duplicated between the USE_COLORS and non-USE_COLORS branches. The only differences are the output function (print vs log.info) and bullet character (• vs -). Consider extracting this logic into a helper function or loop through the folders once and format the output based on USE_COLORS inside the loop to reduce duplication and improve maintainability.
| for folder in plan_entry["folders"]: | |
| # Sanitize folder name and ensure rule count is an integer | |
| raw_name = folder.get("name", "Unknown") | |
| safe_name = sanitize_for_log(raw_name) | |
| rule_count = int(folder.get("rules", 0)) | |
| print(f" • {safe_name} ({rule_count:,} rules)") | |
| print("") | |
| else: | |
| log.info(f"Plan for profile {safe_profile_id}:") | |
| for folder in plan_entry["folders"]: | |
| # Sanitize folder name and ensure rule count is an integer | |
| raw_name = folder.get("name", "Unknown") | |
| safe_name = sanitize_for_log(raw_name) | |
| rule_count = int(folder.get("rules", 0)) | |
| log.info(f" - {safe_name} ({rule_count:,} rules)") | |
| output_func = print | |
| bullet = "•" | |
| else: | |
| log.info(f"Plan for profile {safe_profile_id}:") | |
| output_func = log.info | |
| bullet = "-" | |
| for folder in plan_entry["folders"]: | |
| # Sanitize folder name and ensure rule count is an integer | |
| raw_name = folder.get("name", "Unknown") | |
| safe_name = sanitize_for_log(raw_name) | |
| rule_count = int(folder.get("rules", 0)) | |
| output_func(f" {bullet} {safe_name} ({rule_count:,} rules)") | |
| if USE_COLORS: | |
| print("") |
| safe_profile_id = sanitize_for_log(profile_id) | ||
| if USE_COLORS: | ||
| print(f"\n{Colors.CYAN}🔍 Plan for profile {safe_profile_id}:{Colors.ENDC}") | ||
| for folder in plan_entry["folders"]: | ||
| # Sanitize folder name and ensure rule count is an integer | ||
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| print(f" • {safe_name} ({rule_count:,} rules)") | ||
| print("") | ||
| else: | ||
| log.info(f"Plan for profile {safe_profile_id}:") | ||
| for folder in plan_entry["folders"]: | ||
| # Sanitize folder name and ensure rule count is an integer | ||
| raw_name = folder.get("name", "Unknown") | ||
| safe_name = sanitize_for_log(raw_name) | ||
| rule_count = int(folder.get("rules", 0)) | ||
| log.info(f" - {safe_name} ({rule_count:,} rules)") | ||
|
|
There was a problem hiding this comment.
The new dry-run output functionality lacks test coverage. Consider adding tests that verify the dry-run output is printed correctly for both USE_COLORS=True and USE_COLORS=False scenarios, including proper sanitization of profile IDs and folder names.
…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>
…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 enhances the UX of the
--dry-runmode by printing a detailed plan of the folders and rules that would be created. This allows users to verify the configuration before running the actual sync. Additionally, it cleans up the codebase by removing a duplicate function definition.PR created automatically by Jules for task 16898864247899382848 started by @abhimehro