refactor : added PLC0415 ruff config for specific files #2299
Conversation
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
It's a good start, but the job is just half-way done. Please make sure we use local imports where they are needed only.
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
WalkthroughRemoved local/inline imports and moved them to module scope across AI utilities and tests; migrated Ruff PLC0415 from a global ignore to targeted per-file ignores in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Notes for review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
|
![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.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/pyproject.toml (1)
101-106: Consider consolidating PLC0415 into existing wildcard patterns.The existing per-file patterns
**/models/*.py(line 101) and**/settings/*.py(line 104) currently omit PLC0415, while some of the newly added per-file entries (e.g.,apps/github/models/mixins/organization.py,backend/settings/urls.py) would fall under these wildcards.If all or most files under
**/models/*.pyand**/settings/*.pylegitimately require PLC0415 exceptions, consider adding PLC0415 to those patterns instead of maintaining explicit per-file entries. This reduces duplication and improves maintainability. However, if only a subset justifies the exception, keep the explicit entries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/pyproject.toml(1 hunks)
🔇 Additional comments (1)
backend/pyproject.toml (1)
119-167: Verify coverage: PR description claims 28 files, but only 16 explicit per-file entries are present.The PR objectives state: "Found 51 PLC0415 errors across 28 files" and "Added per-file
ignore = ["PLC0415"]entries for those 28 files." However, the changes show only 16 explicit per-file ignore entries for PLC0415 (lines 119–167).Additionally, the CodeRabbitAI bot's previous scan identified 7 files with indented imports that are not covered by these new entries:
- 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: Some of these would theoretically match existing wildcard patterns (
**/models/*.py,**/settings/*.py), but those patterns do not include PLC0415 in their ignore lists. Consider either:
- Consolidating PLC0415 into the appropriate wildcard patterns (if all files under those patterns justify it), or
- Adding explicit per-file entries for the 7 uncovered files.
Please verify that all 51 PLC0415 errors are now covered by the ruff configuration after these changes, either by explicit per-file entries or by wildcard patterns. You can run the verification script below to confirm no PLC0415 violations remain:
2 participants
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.