Skip to content

🧹 [code health improvement] remove sys.path boilerplate in Scripts by converting to python package #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Ven0m0 wants to merge 1 commit into from
Closed

🧹 [code health improvement] remove sys.path boilerplate in Scripts by converting to python package #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/update-lists.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ jobs:
id: update
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
id: update
id: update
env:
PYTHONPATH: ${{ github.workspace }}

Copilot uses AI. Check for mistakes.
run: |
set -e; mkdir -p lists/mirror
uv run python3 -OO Scripts/update-lists.py --output-dir lists/mirror --max-concurrent 8
uv run python3 -OO -m Scripts.update_lists --output-dir lists/mirror --max-concurrent 8
- name: Run dead domains linter
if: success()
continue-on-error: true
Expand Down
Empty file added Scripts/__init__.py
View file
Open in desktop
Empty file.
6 changes: 1 addition & 5 deletions Scripts/deduplicate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,7 @@
from dataclasses import dataclass
from collections.abc import Iterable

# Add current directory to path to allow importing common if run from elsewhere
if str(Path(__file__).parent) not in sys.path:
sys.path.append(str(Path(__file__).parent))

from common import is_valid_domain, write_lines
from Scripts.common import is_valid_domain, write_lines
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.


@dataclass(slots=True)
Expand Down
6 changes: 1 addition & 5 deletions Scripts/move-pure-domains.py → Scripts/move_pure_domains.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@
from pathlib import Path
from collections import defaultdict

# Import common utilities
if str(Path(__file__).parent) not in sys.path:
sys.path.append(str(Path(__file__).parent))

from common import is_valid_domain, read_lines, write_lines
from Scripts.common import is_valid_domain, read_lines, write_lines


def is_pure_domain(line: str) -> bool:
Expand Down
8 changes: 2 additions & 6 deletions Scripts/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@
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


class TestCommon(unittest.TestCase):
Expand Down Expand Up @@ -89,7 +85,7 @@ def test_is_adguard_rule(self):

def test_write_lines_atomic(self):
import tempfile
from common import write_lines
from Scripts.common import write_lines
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


with tempfile.TemporaryDirectory() as temp_dir:
temp_dir_path = Path(temp_dir)
Expand Down
6 changes: 1 addition & 5 deletions Scripts/test_deduplicate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
import sys
from pathlib import Path
Comment on lines 2 to 3
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sys and Path are unused in this test after removing the sys.path manipulation. Dropping the unused imports avoids lint noise.

Suggested change
import sys
from pathlib import Path

Copilot uses AI. Check for mistakes.

# Add current directory to path to allow importing deduplicate
if str(Path(__file__).parent) not in sys.path:
sys.path.append(str(Path(__file__).parent))

from deduplicate import process_content
from Scripts.deduplicate import process_content


class TestDeduplicate(unittest.TestCase):
Expand Down
10 changes: 2 additions & 8 deletions Scripts/test_is_pure_domain_logic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,11 @@
from pathlib import Path
import importlib

# Add Scripts to path
scripts_dir = Path(__file__).parent
if str(scripts_dir) not in sys.path:
sys.path.append(str(scripts_dir))

# Import ADGUARD_INDICATORS from common
from common import ADGUARD_INDICATORS
from Scripts.common import ADGUARD_INDICATORS

# Import is_pure_domain from move-pure-domains
move_pure_domains = importlib.import_module("move-pure-domains")
is_pure_domain = move_pure_domains.is_pure_domain
from Scripts.move_pure_domains import is_pure_domain


class TestIsPureDomain(unittest.TestCase):
Expand Down
2 changes: 1 addition & 1 deletion Scripts/test_move_pure_domains.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from unittest.mock import Mock, mock_open

# Import the module dynamically
file_path = Path(__file__).parent / "move-pure-domains.py"
file_path = Path(__file__).parent / "move_pure_domains.py"
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

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")
Expand Down
2 changes: 1 addition & 1 deletion Scripts/test_update_lists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class ClientError(Exception):
sys.modules["aiohttp"] = aiohttp_mock
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")
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
update_lists = importlib.util.module_from_spec(spec)
sys.modules["update_lists"] = update_lists
spec.loader.exec_module(update_lists)
Expand Down
6 changes: 1 addition & 5 deletions Scripts/update-lists.py → Scripts/update_lists.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@
import aiohttp
import aiofiles

# Import common utilities
if str(Path(__file__).parent) not in sys.path:
sys.path.append(str(Path(__file__).parent))

from common import sanitize_filename
from Scripts.common import sanitize_filename

# ============================================================================
# CONFIGURATION
Expand Down
Loading