🧹 [code health] Centralize sys.path modifications#147
Conversation
Co-authored-by: Ven0m0 <82972344+Ven0m0@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. |
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, 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 code health of the 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
AnalysisThis is a clean refactoring PR that:
No security concerns, no runtime errors, and no logic bugs detected. The changes improve code maintainability by centralizing module path management. |
![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 is a great code health improvement. Centralizing the sys.path modification into Scripts/__init__.py effectively removes boilerplate from multiple scripts and improves maintainability by making the Scripts directory a proper Python package. The fix to the failing test in test_common.py is also correct. I have one minor suggestion to make the new path handling even more robust.
| @@ -0,0 +1,5 @@ | |||
| import sys | |||
| from pathlib import Path | |||
| scripts_dir = Path(__file__).parent | |||
There was a problem hiding this comment.
For greater robustness, it's a good practice to resolve the path to an absolute path before adding it to sys.path. This ensures that you're always working with a canonical path, which prevents potential issues related to the current working directory, relative paths, or symlinks.
| scripts_dir = Path(__file__).parent | |
| scripts_dir = Path(__file__).parent.resolve() |
There was a problem hiding this comment.
Pull request overview
This PR reduces duplicated Python import-path bootstrapping in Scripts/ by removing per-file sys.path manipulation and updates a filename-sanitization test to match current behavior.
Changes:
- Removed repeated
sys.path.append(...)setup from multipleScripts/*.pyscripts and tests. - Added
Scripts/__init__.pyintended to centralizesys.pathhandling. - Fixed
Scripts/test_common.pyto expect ports to be sanitized with-(not:) in generated filenames.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Scripts/update-lists.py | Removes local sys.path bootstrapping before importing common. |
| Scripts/move-pure-domains.py | Removes local sys.path bootstrapping before importing common. |
| Scripts/deduplicate.py | Removes local sys.path bootstrapping before importing common. |
| Scripts/test_common.py | Removes local sys.path bootstrapping; fixes expected sanitized filename for URLs with ports. |
| Scripts/test_deduplicate.py | Removes local sys.path bootstrapping for test imports. |
| Scripts/test_is_pure_domain_logic.py | Removes local sys.path bootstrapping for test imports. |
| Scripts/init.py | Adds package initializer that mutates sys.path to include Scripts/. |
| import unittest | ||
| import sys | ||
| from pathlib import Path | ||
| import importlib | ||
|
|
There was a problem hiding this comment.
After removing the sys.path bootstrap, sys and Path are unused in this test module. Please remove the unused imports so ruff/linters don’t flag F401 and to reduce noise.
| import sys | ||
| from pathlib import Path | ||
| scripts_dir = Path(__file__).parent | ||
| if str(scripts_dir) not in sys.path: | ||
| sys.path.insert(0, str(scripts_dir)) |
There was a problem hiding this comment.
Scripts/init.py introduces a side-effectful sys.path mutation (inserting the Scripts dir at index 0). This can unexpectedly shadow/import the wrong modules (e.g., a third-party common module) and can also lead to duplicate module loads (common vs Scripts.common) if anything ever imports the package. Also, for the stated invocation python -m unittest discover Scripts ..., this init.py is typically not imported at all (tests are imported as top-level modules), so it likely doesn’t achieve the intended centralization. Prefer removing this sys.path injection (and possibly the file), or switching scripts/tests to package/relative imports (e.g., from .common ... / from Scripts.common ...) and running them via python -m when needed.
| import sys | |
| from pathlib import Path | |
| scripts_dir = Path(__file__).parent | |
| if str(scripts_dir) not in sys.path: | |
| sys.path.insert(0, str(scripts_dir)) | |
| """Package initializer for the Scripts module. | |
| This file is intentionally kept free of side effects such as mutating | |
| sys.path. Importers should rely on standard Python packaging and | |
| relative imports instead of path manipulation. | |
| """ |
| import unittest | ||
| import sys | ||
| from pathlib import Path | ||
| import hashlib |
There was a problem hiding this comment.
After removing the sys.path bootstrap code, sys and Path are no longer used in this test file. Please drop these unused imports to avoid ruff/linters failing on F401 and to keep the tests minimal.
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
After removing the sys.path bootstrap, sys and Path are no longer used here. Please remove the unused imports to avoid ruff/linters reporting F401 and to keep the test focused.
| import sys | |
| from pathlib import Path |
🎯 What: The duplicated
sys.path.appendboilerplate across 6 python scripts in theScripts/directory was removed. A singleScripts/__init__.pyfile was created to handle injecting the package directory intosys.path. A failing test inScripts/test_common.pyregarding filename colons replacement was also fixed to properly expect hyphens.💡 Why: Modifying
sys.pathdynamically in every file creates unnecessary duplication and reduces readability. By creating an__init__.pyand converting the folder into a python package, the Python import system automatically handles module resolution duringunittest discover. When executing scripts directly (e.g.python Scripts/update-lists.py), thesys.pathalready includes the execution directory, making the boilerplate redundant.✅ Verification: I ran
python3 -m unittest discover Scripts/ 'test_*.py'and verified all 28 tests pass successfully. I also ran direct script execution such aspython3 Scripts/test_common.pyto ensure standalone execution works natively without the boilerplate.✨ Result: Improved maintainability, cleaner code, and 1 failing test fixed.
PR created automatically by Jules for task 10027454394505962663 started by @Ven0m0