🧹 [code health improvement] remove sys.path boilerplate in Scripts by converting to python package#150
Conversation
… converting to python package 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 is currently in state
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 Python scripts by refactoring their import mechanisms. It transitions the Highlights
Changelog
Ignored Files
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
|
📝 WalkthroughWalkthroughThe PR standardizes module imports across the Scripts directory by replacing relative imports and sys.path manipulation with absolute imports using the Scripts package structure. Configuration is also updated to invoke scripts as Python modules. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 is a great step towards improving the Python code health by converting the Scripts directory into a proper package and removing sys.path manipulations. The changes are clean and follow the stated goal.
I've added a few suggestions, primarily in the test files, to further leverage this new package structure. By replacing dynamic importlib loading with standard, direct imports, the tests can be made simpler, cleaner, and more idiomatic. These are follow-up improvements that this refactoring enables.
| def test_write_lines_atomic(self): | ||
| import tempfile | ||
| from common import write_lines | ||
| from Scripts.common import write_lines |
There was a problem hiding this comment.
To adhere to PEP 8, this local import should be moved to the top of the file. Please add write_lines to the import on line 6 and remove this line.
|
|
||
| # Import the module dynamically | ||
| file_path = Path(__file__).parent / "move-pure-domains.py" | ||
| file_path = Path(__file__).parent / "move_pure_domains.py" |
There was a problem hiding this comment.
Now that Scripts is a package, this dynamic import is no longer necessary. The entire block from lines 6-14 can be replaced with a simple from Scripts.move_pure_domains import is_pure_domain, scan_adblock_files. This would make the test cleaner and more idiomatic.
| sys.modules["aiofiles"] = MagicMock() | ||
|
|
||
| spec = importlib.util.spec_from_file_location("update_lists", "Scripts/update-lists.py") | ||
| spec = importlib.util.spec_from_file_location("update_lists", "Scripts/update_lists.py") |
There was a problem hiding this comment.
This dynamic import and the sys.modules patching above are no longer needed. You can refactor this to use a direct import (from Scripts import update_lists) and then use unittest.mock.patch decorators on the test class to mock aiohttp and aiofiles. This is a more standard and robust way to handle test dependencies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Scripts/test_move_pure_domains.py (1)
2-14:⚠️ Potential issue | 🟠 MajorUse standard package imports instead of dynamic file loading.
The test loads the module via
spec_from_file_location(), which bypasses the packaged import path. Sincemove_pure_domains.pynow uses absolute imports (from Scripts.common), this test won't catch if those imports are broken. Import viaScripts.move_pure_domainsso the test actually validates the refactor.Suggested change
-import importlib.util -from pathlib import Path from unittest.mock import Mock, mock_open - -# Import the module dynamically -file_path = Path(__file__).parent / "move_pure_domains.py" -spec = importlib.util.spec_from_file_location("move_pure_domains", file_path) -if spec is None or spec.loader is None: - raise ImportError("Could not load module") -module = importlib.util.module_from_spec(spec) -spec.loader.exec_module(module) -is_pure_domain = module.is_pure_domain -scan_adblock_files = module.scan_adblock_files +from Scripts.move_pure_domains import is_pure_domain, scan_adblock_files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/test_move_pure_domains.py` around lines 2 - 14, Replace the dynamic file-loading block with a standard package import so tests exercise the module's absolute imports: import the module via "import Scripts.move_pure_domains as move_pure_domains" (or "from Scripts.move_pure_domains import is_pure_domain, scan_adblock_files") instead of using importlib.util.spec_from_file_location, then reference the functions is_pure_domain and scan_adblock_files from that imported module to ensure the package-level imports are validated.
🧹 Nitpick comments (3)
Scripts/test_deduplicate.py (1)
2-3: Unused imports.
sysandPathappear to be unused after removing thesys.pathmanipulation. These can be cleaned up, though this is minor.Proposed fix
import unittest -import sys -from pathlib import Path from Scripts.deduplicate import process_content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/test_deduplicate.py` around lines 2 - 3, Remove the unused imports by deleting the now-unreferenced "import sys" and "from pathlib import Path" lines in Scripts/test_deduplicate.py so only necessary imports remain; search for any remaining usage of sys or Path (none expected) and run tests/lint to confirm no breakage.Scripts/test_is_pure_domain_logic.py (1)
2-4: Unused imports.
sys,Path, andimportlibappear unused after removing the dynamic import logic.Proposed fix
import unittest -import sys -from pathlib import Path -import importlib # Import ADGUARD_INDICATORS from common from Scripts.common import ADGUARD_INDICATORS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/test_is_pure_domain_logic.py` around lines 2 - 4, Remove the now-unused imports sys, Path, and importlib from the test module top-level imports (they were left after removing the dynamic import logic); search the file for any remaining uses of sys, Path, or importlib to confirm nothing else relies on them, delete those import lines (e.g., the import statements referencing sys, Path, importlib), and run the tests/linter to ensure no remaining references remain.Scripts/test_common.py (1)
2-2: Unusedsysimport.
sysis no longer used after removing the path manipulation.Proposed fix
import unittest -import sys from pathlib import Path🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/test_common.py` at line 2, Remove the unused top-level import by deleting the "import sys" statement in the module (Scripts/test_common.py); ensure no other references to sys remain and run the linter/tests to confirm no import-related errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Scripts/test_move_pure_domains.py`:
- Around line 2-14: Replace the dynamic file-loading block with a standard
package import so tests exercise the module's absolute imports: import the
module via "import Scripts.move_pure_domains as move_pure_domains" (or "from
Scripts.move_pure_domains import is_pure_domain, scan_adblock_files") instead of
using importlib.util.spec_from_file_location, then reference the functions
is_pure_domain and scan_adblock_files from that imported module to ensure the
package-level imports are validated.
---
Nitpick comments:
In `@Scripts/test_common.py`:
- Line 2: Remove the unused top-level import by deleting the "import sys"
statement in the module (Scripts/test_common.py); ensure no other references to
sys remain and run the linter/tests to confirm no import-related errors.
In `@Scripts/test_deduplicate.py`:
- Around line 2-3: Remove the unused imports by deleting the now-unreferenced
"import sys" and "from pathlib import Path" lines in Scripts/test_deduplicate.py
so only necessary imports remain; search for any remaining usage of sys or Path
(none expected) and run tests/lint to confirm no breakage.
In `@Scripts/test_is_pure_domain_logic.py`:
- Around line 2-4: Remove the now-unused imports sys, Path, and importlib from
the test module top-level imports (they were left after removing the dynamic
import logic); search the file for any remaining uses of sys, Path, or importlib
to confirm nothing else relies on them, delete those import lines (e.g., the
import statements referencing sys, Path, importlib), and run the tests/linter to
ensure no remaining references remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 449088d6-79eb-4d77-883d-edbcc3ccca31
📒 Files selected for processing (10)
.github/workflows/update-lists.ymlScripts/__init__.pyScripts/deduplicate.pyScripts/move_pure_domains.pyScripts/test_common.pyScripts/test_deduplicate.pyScripts/test_is_pure_domain_logic.pyScripts/test_move_pure_domains.pyScripts/test_update_lists.pyScripts/update_lists.py
There was a problem hiding this comment.
Pull request overview
This PR converts the Scripts/ directory into a proper Python package to eliminate per-script sys.path manipulation and enable standard package imports across the repo.
Changes:
- Added
Scripts/__init__.pyand updated Python modules to import shared helpers viaScripts.common. - Renamed hyphenated script filenames to snake_case and updated tests accordingly.
- Updated the
update-listsGitHub Action to run the updater viapython -m Scripts.update_lists.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Scripts/update_lists.py | Switches common utility import to Scripts.common. |
| Scripts/move_pure_domains.py | Switches common utility imports to Scripts.common. |
| Scripts/deduplicate.py | Switches common utility imports to Scripts.common. |
| Scripts/init.py | Marks Scripts/ as a Python package. |
| Scripts/test_update_lists.py | Updates dynamic import path to renamed file. |
| Scripts/test_move_pure_domains.py | Updates dynamic import path to renamed file. |
| Scripts/test_is_pure_domain_logic.py | Replaces sys.path hacking / dynamic import with package imports. |
| Scripts/test_deduplicate.py | Replaces sys.path hacking with package import. |
| Scripts/test_common.py | Replaces sys.path hacking with package import. |
| .github/workflows/update-lists.yml | Runs updater with python -m Scripts.update_lists. |
Comments suppressed due to low confidence (2)
Scripts/update_lists.py:23
from Scripts.common ...will fail when this file is executed directly (e.g.python3 Scripts/update_lists.pyor via the shebang) becausesys.path[0]becomes theScripts/directory, so the top-levelScriptspackage is not importable. To avoid breaking direct execution, either switch to intra-package relative imports (and requirepython3 -m Scripts.update_lists), or add a small compatibility import fallback/entrypoint that doesn’t rely on mutatingsys.path.
Scripts/move_pure_domains.py:11- Like the other Scripts utilities, this absolute import (
from Scripts.common ...) will fail if the file is executed directly (e.g.python3 Scripts/move_pure_domains.py) because the repo root is not onsys.path. If the intent is to require module execution, prefer package-relative imports and ensure all entrypoints usepython3 -m Scripts.move_pure_domains; otherwise add a compatibility import/entrypoint.
| import sys | ||
| from pathlib import Path | ||
| import importlib | ||
|
|
There was a problem hiding this comment.
sys, Path, and importlib are now unused after switching to direct package imports. Removing these imports will prevent unused-import lint failures and keeps the test focused.
| import sys | ||
| from pathlib import Path |
There was a problem hiding this comment.
sys and Path are unused in this test after removing the sys.path manipulation. Dropping the unused imports avoids lint noise.
| import sys | |
| from pathlib import Path |
| import sys | ||
| from pathlib import Path | ||
| import hashlib | ||
|
|
||
| # Add current directory to path | ||
| if str(Path(__file__).parent) not in sys.path: | ||
| sys.path.append(str(Path(__file__).parent)) | ||
|
|
||
| from common import sanitize_filename, is_valid_domain, is_adguard_rule | ||
| from Scripts.common import sanitize_filename, is_valid_domain, is_adguard_rule |
There was a problem hiding this comment.
sys is no longer used in this test file; consider removing the import to avoid unused-import lint warnings.
| sys.path.append(str(Path(__file__).parent)) | ||
|
|
||
| from common import is_valid_domain, write_lines | ||
| from Scripts.common import is_valid_domain, write_lines |
There was a problem hiding this comment.
This new absolute import (from Scripts.common ...) will break existing direct invocations like python3 Scripts/deduplicate.py (see run_pre_commit.sh and Scripts/README.md), raising ModuleNotFoundError: No module named 'Scripts' because the repo root isn’t on sys.path for file execution. Consider converting this script to package-relative imports and updating the repo to run it as a module (python3 -m Scripts.deduplicate), or otherwise provide a backward-compatible entrypoint.
| from Scripts.common import is_valid_domain, write_lines | |
| try: | |
| from Scripts.common import is_valid_domain, write_lines | |
| except ModuleNotFoundError: | |
| from common import is_valid_domain, write_lines |
| @@ -46,7 +46,7 @@ jobs: | |||
| id: update | |||
There was a problem hiding this comment.
With PYTHONSAFEPATH: 1 set for the job, python -m Scripts.update_lists may not be able to import the local Scripts package unless the repo root is explicitly on sys.path (e.g. via installing the project into the venv or setting PYTHONPATH=$GITHUB_WORKSPACE). If this step fails with No module named Scripts, consider unsetting PYTHONSAFEPATH for this step, adding PYTHONPATH, or ensuring uv sync installs the project/package so the module is found in site-packages.
| id: update | |
| id: update | |
| env: | |
| PYTHONPATH: ${{ github.workspace }} |
| sys.modules["aiofiles"] = MagicMock() | ||
|
|
||
| spec = importlib.util.spec_from_file_location("update_lists", "Scripts/update-lists.py") | ||
| spec = importlib.util.spec_from_file_location("update_lists", "Scripts/update_lists.py") |
There was a problem hiding this comment.
This test loads the module using a relative string path ("Scripts/update_lists.py"), which makes the test sensitive to the current working directory. Use Path(__file__).parent / "update_lists.py" (as done in other tests) so it runs reliably regardless of where unittest is invoked from.
| spec = importlib.util.spec_from_file_location("update_lists", "Scripts/update_lists.py") | |
| module_path = Path(__file__).parent / "update_lists.py" | |
| spec = importlib.util.spec_from_file_location("update_lists", module_path) |
🎯 What: Removed repetitive
sys.path.append(str(Path(__file__).parent))blocks from Python files in theScripts/directory. Created an__init__.pyto makeScriptsa proper Python module, enabling standard absolute imports (e.g.,from Scripts.common import ...). Renamed files containing hyphens to snake_case for proper python package compatibility.💡 Why: Having each file defensively modify the execution environment (
sys.path) to load sibling files is brittle and creates messy boilerplate. Establishing a clear package structure (Scripts) ensures stable relative/absolute imports across the project without runtime path hacking.✅ Verification: Verified by running the python unit test suite (
python3 -m unittest discover Scripts/ 'test_*.py') and confirming that GitHub workflow.github/workflows/update-lists.ymlcontinues to execute the scripts flawlessly using standard module execution syntax (python3 -m Scripts.update_lists).✨ Result: A cleaner codebase with standard Python import practices, resolving path resolution issues and removing unnecessary repetitive code blocks.
PR created automatically by Jules for task 6564146397324574189 started by @Ven0m0
Summary by CodeRabbit