refactor : added PLC0415 ruff config for specific files #2299
Conversation
backend/pyproject.toml
Outdated
| "DJ012", # https://docs.astral.sh/ruff/rules/django-unordered-body-content-in-model/ | ||
| "FIX002", # https://docs.astral.sh/ruff/rules/line-contains-todo/ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| "ANN", # https://docs.astral.sh/ruff/rules/#flake8-annotations-ann/ |
There was a problem hiding this comment.
Please keep the rest unchanged, revert the formatting changes.
backend/pyproject.toml
Outdated
| lint.per-file-ignores."apps/slack/utils.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."tests/apps/ai/management/commands/ai_update_chapter_chunks_test.py" = [ |
There was a problem hiding this comment.
You need to verify that all non top-level imports are justified. The idea is to minimize them to must have cases only. This one, for example, looks like a good candidate for the removal.
There was a problem hiding this comment.
@arkid15r Sorry for the inefficient approach earlier.
Just to confirm—should I go ahead and move the imports to the top of the files (in cases where having them inside classes/functions isn’t justified), and then remove PLC0415 from the ignore list?
There was a problem hiding this comment.
Yes, for the cases where it's just an easy move to the top -- please do it. No all cases can be fixed though, so you will still have to keep some per file ignores, mostly for slack app.
Summary by CodeRabbit
WalkthroughEnables Ruff PLC0415 rule globally by updating backend/pyproject.toml and adds targeted per-file ignores. Consolidates local imports to module-level in ai utils and multiple test modules by moving imports to the top of files. No logic or public API changes; tests updated for import locality only. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/tests/apps/github/management/commands/github_get_installation_id_test.py (2)
77-82: Harden against env-based flakiness by clearing GITHUB_APP_ID.If GITHUB_APP_ID is set in the environment, this test will fail spuriously. Clear env for determinism.
Apply this diff:
- with pytest.raises(SystemExit): - command.handle() + with mock.patch.dict(os.environ, {}, clear=True), pytest.raises(SystemExit): + command.handle()
102-116: Configure mock return to avoid iterating over a bare MagicMock.handle() does list(gi.get_installations()). Without a configured return, iterating a MagicMock can raise TypeError. Configure it to return [] and assert the call.
Apply this diff:
command = Command() with ( mock.patch("pathlib.Path.open", mock.mock_open(read_data=self.test_private_key)), mock.patch("pathlib.Path.exists", return_value=True), - mock.patch( - "apps.github.management.commands.github_get_installation_id.GithubIntegration" - ), - mock.patch("apps.github.management.commands.github_get_installation_id.Auth.AppAuth"), + mock.patch( + "apps.github.management.commands.github_get_installation_id.GithubIntegration" + ) as mock_github_integration, + mock.patch("apps.github.management.commands.github_get_installation_id.Auth.AppAuth"), ): - command.handle(app_id=123456, private_key_file=custom_key_path) + mock_gi_instance = mock.MagicMock() + mock_gi_instance.get_installations.return_value = [] + mock_github_integration.return_value = mock_gi_instance + + command.handle(app_id=123456, private_key_file=custom_key_path) + + mock_gi_instance.get_installations.assert_called_once()
🧹 Nitpick comments (4)
backend/tests/apps/ai/management/commands/ai_update_chapter_context_test.py (2)
1-1: Fix docstring to reflect “update” command.Adjust to avoid confusion with the current command name.
-"""Tests for the ai_create_chapter_context Django management command.""" +"""Tests for the ai_update_chapter_context Django management command."""
25-25: Rename test class for consistency with the command name.Keeps naming aligned with the updated command.
-class TestAiCreateChapterContextCommand: +class TestAiUpdateChapterContextCommand:backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (1)
1-1: Rename “create” → “update” in docstrings/class to match the command name.Cosmetic but reduces confusion.
Apply this diff:
-"""Tests for the ai_create_committee_context command.""" +"""Tests for the ai_update_committee_context command.""" @@ -class TestAiCreateCommitteeContextCommand: - """Test suite for the ai_create_committee_context command.""" +class TestAiUpdateCommitteeContextCommand: + """Test suite for the ai_update_committee_context command."""Also applies to: 30-31
backend/pyproject.toml (1)
122-170: Optional: add a short rationale/TODO per persistent ignore.To guide future cleanups, consider a brief inline note on why each file must keep late imports (e.g., circular deps, heavy/conditional imports). Helps ensure these ignores don’t linger indefinitely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/ai/common/utils.py(0 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/ai/management/commands/ai_update_chapter_chunks_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_chapter_context_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_committee_chunks_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_event_chunks_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_event_context_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_project_chunks_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_project_context_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_slack_message_chunks_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_slack_message_context_test.py(1 hunks)backend/tests/apps/github/management/commands/github_get_installation_id_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- backend/apps/ai/common/utils.py
🧰 Additional context used
🧬 Code graph analysis (9)
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (2)
backend/apps/ai/common/base/context_command.py (1)
BaseContextCommand(9-54)backend/apps/ai/management/commands/ai_update_committee_context.py (1)
Command(8-14)
backend/tests/apps/ai/management/commands/ai_update_committee_chunks_test.py (1)
backend/apps/owasp/api/internal/queries/committee.py (1)
committee(14-28)
backend/tests/apps/ai/management/commands/ai_update_slack_message_chunks_test.py (1)
backend/apps/slack/models/message.py (1)
Message(15-160)
backend/tests/apps/ai/management/commands/ai_update_project_context_test.py (2)
backend/apps/ai/common/base/context_command.py (1)
BaseContextCommand(9-54)backend/apps/ai/management/commands/ai_update_project_context.py (1)
Command(10-20)
backend/tests/apps/ai/management/commands/ai_update_chapter_chunks_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/tests/apps/github/management/commands/github_get_installation_id_test.py (1)
backend/apps/github/management/commands/github_get_installation_id.py (1)
Command(15-113)
backend/tests/apps/ai/management/commands/ai_update_chapter_context_test.py (2)
backend/apps/ai/common/base/context_command.py (1)
BaseContextCommand(9-54)backend/apps/ai/management/commands/ai_update_chapter_context.py (1)
Command(8-14)
backend/tests/apps/ai/management/commands/ai_update_slack_message_context_test.py (2)
backend/apps/ai/common/base/context_command.py (1)
BaseContextCommand(9-54)backend/apps/slack/models/message.py (1)
Message(15-160)
backend/tests/apps/ai/management/commands/ai_update_event_context_test.py (2)
backend/apps/ai/common/base/context_command.py (1)
BaseContextCommand(9-54)backend/apps/ai/management/commands/ai_update_event_context.py (1)
Command(10-24)
🔇 Additional comments (12)
backend/tests/apps/ai/management/commands/ai_update_chapter_context_test.py (1)
7-9: Top-level imports look good for PLC0415; confirm Django settings are initialized during test collection.Moving these imports to module scope aligns with enabling PLC0415. Please confirm pytest collection works in isolation for this file, since importing Django models at module import time can require pytest-django settings to be initialized.
backend/tests/apps/ai/management/commands/ai_update_committee_chunks_test.py (1)
9-9: Module‑scope import for Committee: LGTM for PLC0415.Consistent with the PR’s strategy; no behavior change.
backend/tests/apps/ai/management/commands/ai_update_project_chunks_test.py (1)
7-7: Module‑scope import for Project: Looks good.Matches the PLC0415 cleanup approach.
backend/tests/apps/ai/management/commands/ai_update_slack_message_context_test.py (1)
5-7: Promoting BaseContextCommand and Message imports to module scope: LGTM.Keeps tests compliant with PLC0415 without affecting assertions.
backend/tests/apps/ai/management/commands/ai_update_chapter_chunks_test.py (1)
7-7: Module‑scope import for Chapter: LGTM.Aligns with PLC0415 enablement.
backend/tests/apps/ai/management/commands/ai_update_slack_message_chunks_test.py (1)
7-7: Module‑level Message import: LGTM.Consistent with the rest of the suite and PLC0415.
backend/tests/apps/ai/management/commands/ai_update_event_context_test.py (1)
7-9: Module‑scope imports (BaseContextCommand, Event): LGTM.Compliant with PLC0415; assertions remain unchanged.
backend/tests/apps/ai/management/commands/ai_update_project_context_test.py (1)
5-7: Module‑scope imports (BaseContextCommand, Project): LGTM.Matches the refactor pattern across tests.
backend/tests/apps/ai/management/commands/ai_update_event_chunks_test.py (1)
9-9: Module‑scope import for Event: LGTM and aligns with PLC0415.No indented/non‑top‑level imports found in backend/tests/**; pyproject.toml has PLC0415 per‑file‑ignores only for specific files (not this test).
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (1)
7-7: Module-level imports OK — pytest-django configuredbackend/pyproject.toml sets [tool.pytest.ini_options] with DJANGO_SETTINGS_MODULE="settings.test" and DJANGO_CONFIGURATION="Test", and pytest-django is present in dependencies (poetry.lock) — Django will be initialized before test-module import.
backend/tests/apps/github/management/commands/github_get_installation_id_test.py (1)
9-10: Module-level import aligns with PLC0415 objective.Good move pulling the import to the top-level; this removes the import-outside-toplevel warning for this file.
backend/pyproject.toml (1)
162-170: Confirm PLC0415 per-file ignores exist and cover all affected testsI didn't find any 'lint.per-file-ignores.*PLC0415' entries in backend/pyproject.toml and the tests/ directory is absent in the sandbox. Confirm the three ignores are present in the branch and add any other test files that trigger PLC0415 to avoid CI failures.
| # specific files that need PLC0415 ignored | ||
| lint.per-file-ignores."apps/github/models/mixins/organization.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/github/models/mixins/user.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/mentorship/apps.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/apps.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/commands/events.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/commands/leaders.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/common/handlers/chapters.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/common/handlers/committees.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/common/handlers/contribute.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/common/handlers/projects.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/common/handlers/users.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/events/member_joined_channel/contribute.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] | ||
| lint.per-file-ignores."apps/slack/utils.py" = [ | ||
| "PLC0415", # https://docs.astral.sh/ruff/rules/import-outside-top-level/ | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
LGTM: targeted PLC0415 per-file ignores look correct.
Entries use valid Ruff v2-style keys (lint.per-file-ignores."...") and are scoped to concrete files, aligning with the objective to remove the global ignore. No nonessential formatting changes observed.
Please confirm:
- All remaining PLC0415 occurrences were either refactored (imports moved to top-level) or are covered by these per-file entries.
- No overlapping E402 violations remain (Ruff can also flag top-level import placement via E402). If any exist, consider aligning on a single rule to suppress.
You can run the following repo-scoped reconnaissance to spot non top-level imports that might have been missed (heuristic; will surface imports inside defs/classes):
🏁 Script executed:
#!/bin/bash
# Heuristic scan for indented imports (potential PLC0415 sites)
rg -nP -C2 '^\s{2,}(?:from\s+\S+\s+import\b|import\s+\S+)\b' -g '!**/migrations/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo "=== git status (short) ==="
git rev-parse --abbrev-ref HEAD || true
git status --porcelain --untracked-files=no || true
echo
echo "=== Searching for literal 'PLC0415' ==="
rg -n --hidden -uu 'PLC0415' || true
echo
echo "=== Searching for literal 'E402' ==="
rg -n --hidden -uu '\bE402\b' || true
echo
echo "=== Heuristic scan for indented imports (exclude migrations) ==="
rg -nP -C2 '^\s{2,}(?:from\s+\S+\s+import\b|import\s+\S+)\b' --hidden -uu --glob '!**/migrations/**' || true
echo
echo "=== Show backend/pyproject.toml (first 300 lines) ==="
if [ -f backend/pyproject.toml ]; then
sed -n '1,300p' backend/pyproject.toml
else
echo "backend/pyproject.toml not found"
fiLength of output: 10422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo
echo "=== ruff.toml (first 200 lines) ==="
if [ -f ruff.toml ]; then sed -n '1,200p' ruff.toml; else echo "ruff.toml not found"; fi
echo
echo "=== backend/pyproject.toml (first 300 lines) ==="
if [ -f backend/pyproject.toml ]; then sed -n '1,300p' backend/pyproject.toml; else echo "backend/pyproject.toml not found"; fi
echo
echo "=== Grep for literal 'PLC0415' and 'E402' ==="
rg -n --hidden -uu 'PLC0415' || true
rg -n --hidden -uu '\bE402\b' || true
echo
echo "=== Scanning .py files for indented imports (heuristic for PLC0415/E402) ==="
find . -type f -name '*.py' \
-not -path '*/.git/*' \
-not -path '*/migrations/*' \
-not -path '*/.venv/*' \
-not -path '*/venv/*' \
-not -path '*/node_modules/*' \
-print0 |
while IFS= read -r -d '' file; do
rg -nP '^\s{2,}(?:from\s+\S+\s+import\b|import\s+\S+)' -S -H "$file" || true
done
echo
echo "=== Done ==="Length of output: 14854
LGTM — per-file PLC0415 ignores look correct, but uncovered PLC0415 sites remain
Repo scan shows additional indented imports not covered by backend/pyproject.toml; move these imports to module top-level or add PLC0415 per-file ignores for the following files:
- backend/apps/github/api/internal/nodes/repository.py
- backend/apps/github/models/organization.py
- backend/apps/nest/models/user.py
- backend/apps/owasp/models/event.py
- backend/apps/slack/models/conversation.py
- backend/manage.py
- backend/settings/urls.py
Note: ruff.toml currently ignores E402 for **/init.py and **/conftest.py — consider aligning E402 vs PLC0415 suppression to avoid overlapping rules.
Proposed change
PLC0415from the global ignore.ruff checkto identify the files triggeringPLC0415.pyproject.tomlfor all 28 files:Resolves #2297
Checklist
make check-testlocally; all checks and tests passed.