🎨 Palette: UX improvements for CLI dry-run and help text#388
Conversation
- Add "Next Steps" footer to dry-run output with copy-pasteable command - Enhance `argparse` help text with friendly description and examples - Fix logic error in `validate_hostname` (missing return True, unreachable code) - Fix flaky and broken tests in `tests/` 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 significantly improves the user experience of the CLI tool and enhances its overall reliability. It introduces a clear 'Next Steps' guide after a dry-run to streamline the process of transitioning to live synchronization, refines the CLI's help text for better clarity and user-friendliness, and addresses a critical bug in hostname validation. Additionally, several test suite issues were resolved to ensure consistent and reliable verification of the application's functionality. 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
|
There was a problem hiding this comment.
Pull request overview
This PR improves the CLI’s user experience by enhancing dry-run output with a “Next Steps” hint, refreshing the help text messaging, and tightening correctness around hostname validation while stabilizing parts of the test suite.
Changes:
- Add a dry-run “Next Steps” section that prints a suggested live-run command.
- Update
argparsehelp description/epilog text to be more user-friendly. - Fix
validate_hostnameto always return a boolean, and adjust tests for rate-limit/jitter behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
main.py |
Adds dry-run “Next Steps” command suggestion, updates CLI help text, and fixes validate_hostname boolean return. |
tests/test_retry_jitter.py |
Ensures mocked 429 responses include headers dict to match production expectations. |
tests/test_rate_limit.py |
Makes jitter deterministic in a 429 backoff test by patching random.random. |
tests/test_push_rules_perf.py |
Removes an unnecessary self.main assignment in setUp. |
main.py
Outdated
| # Construct command suggestion | ||
| cmd_parts = ["python", "main.py"] | ||
| if profile_ids: | ||
| # Join multiple profiles if needed | ||
| p_str = ",".join(profile_ids) | ||
| cmd_parts.append(f"--profiles {p_str}") | ||
| else: | ||
| cmd_parts.append("--profiles <your-profile-id>") | ||
|
|
||
| # Reconstruct other args if they were used (optional but helpful) | ||
| if args.folder_url: | ||
| for url in args.folder_url: | ||
| cmd_parts.append(f"--folder-url {url}") | ||
|
|
||
| cmd_str = " ".join(cmd_parts) | ||
| print(f" {Colors.CYAN}{cmd_str}{Colors.ENDC}") |
There was a problem hiding this comment.
The suggested dry-run command is built by concatenating raw user-provided values (profile IDs / folder URLs) into a single string without shell-quoting or sanitization. This can produce a copy/paste command that breaks for URLs containing special characters/spaces and also risks echoing secrets (e.g., basic-auth or token query params) and terminal control characters into logs/CI output. Consider constructing the command as a list of discrete args (e.g., ['--profiles', p_str], repeated ['--folder-url', url]) and rendering it with shlex.join/shlex.quote; additionally, avoid printing sensitive URL components (or detect/redact and warn when redaction occurs).
![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 several nice UX improvements for the CLI, such as a more friendly help message and a 'Next Steps' guide after a dry run. It also includes important bug fixes for hostname validation and flaky tests. The changes are well-implemented. I've left one suggestion to refactor the new dry-run command construction to fix a minor bug and reduce code duplication. Overall, great work on improving the user experience and robustness of the tool.
| if all_success: | ||
| if USE_COLORS: | ||
| print(f"{Colors.BOLD}👉 Ready to sync? Run the following command:{Colors.ENDC}") | ||
|
|
||
| # Construct command suggestion | ||
| cmd_parts = ["python", "main.py"] | ||
| if profile_ids: | ||
| # Join multiple profiles if needed | ||
| p_str = ",".join(profile_ids) | ||
| cmd_parts.append(f"--profiles {p_str}") | ||
| else: | ||
| cmd_parts.append("--profiles <your-profile-id>") | ||
|
|
||
| # Reconstruct other args if they were used (optional but helpful) | ||
| if args.folder_url: | ||
| for url in args.folder_url: | ||
| cmd_parts.append(f"--folder-url {url}") | ||
|
|
||
| cmd_str = " ".join(cmd_parts) | ||
| print(f" {Colors.CYAN}{cmd_str}{Colors.ENDC}") | ||
| else: | ||
| print("👉 Ready to sync? Run the following command:") | ||
| p_str = ",".join(profile_ids) if profile_ids else "<your-profile-id>" | ||
| print(f" python main.py --profiles {p_str}") | ||
| else: | ||
| if USE_COLORS: | ||
| print( | ||
| f"{Colors.FAIL}⚠️ Dry run encountered errors. Please check the logs above.{Colors.ENDC}" | ||
| ) | ||
| else: | ||
| print("⚠️ Dry run encountered errors. Please check the logs above.") |
There was a problem hiding this comment.
There's a bit of duplicated logic for constructing the command suggestion, and a bug in the else block for when USE_COLORS is false. The command suggestion for the no-color case is missing the --folder-url arguments that might have been passed.
You can refactor this block to build the command string first, and then print it with or without colors. This will fix the bug and make the code DRY (Don't Repeat Yourself).
if all_success:
# Construct command suggestion
cmd_parts = ["python", "main.py"]
if profile_ids:
p_str = ",".join(profile_ids)
cmd_parts.append(f"--profiles {p_str}")
else:
cmd_parts.append("--profiles <your-profile-id>")
# Reconstruct other args if they were used (optional but helpful)
if args.folder_url:
for url in args.folder_url:
cmd_parts.append(f"--folder-url {url}")
cmd_str = " ".join(cmd_parts)
if USE_COLORS:
print(f"{Colors.BOLD}👉 Ready to sync? Run the following command:{Colors.ENDC}")
print(f" {Colors.CYAN}{cmd_str}{Colors.ENDC}")
else:
print("👉 Ready to sync? Run the following command:")
print(f" {cmd_str}")
else:
if USE_COLORS:
print(
f"{Colors.FAIL}⚠️ Dry run encountered errors. Please check the logs above.{Colors.ENDC}"
)
else:
print("⚠️ Dry run encountered errors. Please check the logs above.")Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def test_429_without_retry_after_uses_exponential_backoff(self): | ||
| @patch("random.random", return_value=0.5) | ||
| def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): |
Check warning
Code scanning / Prospector (reported by Codacy)
Unused argument 'mock_random' (unused-argument) Warning test
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (104/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (109/100) Warning
|
|
||
| def test_429_without_retry_after_uses_exponential_backoff(self): | ||
| @patch("random.random", return_value=0.5) | ||
| def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Method could be a function Warning test
|
|
||
| def test_429_without_retry_after_uses_exponential_backoff(self): | ||
| @patch("random.random", return_value=0.5) | ||
| def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Unused argument 'mock_random' Note test
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (104/100) Warning
main.py
Outdated
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (109/100) Warning
|
|
||
| def test_429_without_retry_after_uses_exponential_backoff(self): | ||
| @patch("random.random", return_value=0.5) | ||
| def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): |
Check warning
Code scanning / Pylint (reported by Codacy)
Method could be a function Warning test
|
|
||
| def test_429_without_retry_after_uses_exponential_backoff(self): | ||
| @patch("random.random", return_value=0.5) | ||
| def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): |
Check notice
Code scanning / Pylint (reported by Codacy)
Unused argument 'mock_random' Note test
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
💡 What:
validate_hostnamewhere it could return None (implicitly) instead of True.🎯 Why:
📸 Before/After:
Before: Dry run ended with just a summary table (or error logs).
After: Dry run ends with "👉 Ready to sync? Run: python main.py ..."
♿ Accessibility:
PR created automatically by Jules for task 5033950765328649422 started by @abhimehro