refactor:remove PLR0912 ignore and fix 4 violations update #1979
Conversation
… PLR0912 (too many branches)
Summary by CodeRabbit
WalkthroughThis set of changes refactors several backend modules to address "too-many-branches" (PLR0912) linter violations. Key methods in AI management commands and GitHub repository synchronization are modularized, introducing helper methods and loops to reduce branching complexity. The Ruff linter configuration is updated to enforce PLR0912 by removing it from ignored rules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/github/utils.py (1)
46-67: Consider adding type hints for better clarity.The helper function implementation is correct. Consider adding type hints to improve code documentation and IDE support.
-def _check_single_funding_policy_compliance(platform, target): +def _check_single_funding_policy_compliance(platform: str, target: str) -> bool:backend/apps/ai/management/commands/ai_create_chapter_chunks.py (1)
139-149: Good uniform handling of metadata fields.The approach correctly handles both string and list types. However, consider extracting the list joining logic for reusability.
You could extract the display value logic into a small helper:
def _format_display_value(value): """Format value for display, joining lists with commas.""" return ", ".join(value) if isinstance(value, list) else valueThen use it in the loop:
- display_value = ", ".join(value) if isinstance(value, list) else value + display_value = _format_display_value(value)backend/apps/ai/management/commands/ai_create_project_chunks.py (1)
89-91: Consider using type annotations for clarity.Adding type hints would improve code documentation and IDE support.
- def extract_project_content(self, project: Project) -> tuple[str, str]: - prose_parts: list[str] = [] - metadata_parts: list[str] = [] + def extract_project_content(self, project: Project) -> tuple[str, str]: + prose_parts: list[str] = [] + metadata_parts: list[str] = []Note: The type hints are already present, this is good practice!
backend/apps/github/models/repository.py (1)
366-389: Consider adding error handling for milestone sync.While the UnknownObjectException is caught for labels, consider wrapping the entire milestone update in a try-except to handle potential GitHub API errors gracefully.
def sync_milestones(self, gh_repository): """Sync milestones from GitHub repository.""" until = ( self.latest_updated_milestone.updated_at if self.latest_updated_milestone else timezone.now() - td(days=30) ) for gh_milestone in gh_repository.get_milestones( direction="desc", sort="updated", state="all" ): if gh_milestone.updated_at < until: break + try: milestone = Milestone.update_data( gh_milestone, author=User.update_data(gh_milestone.creator), repository=self, ) milestone.labels.clear() for gh_milestone_label in gh_milestone.get_labels(): try: milestone.labels.add(Label.update_data(gh_milestone_label)) except UnknownObjectException: logger.exception("Couldn't get GitHub milestone label %s", milestone.url) + except GithubException as e: + logger.exception("Failed to sync milestone for repository %s: %s", self.name, str(e)) + continue
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/ai/management/commands/ai_create_chapter_chunks.py(2 hunks)backend/apps/ai/management/commands/ai_create_project_chunks.py(2 hunks)backend/apps/github/common.py(1 hunks)backend/apps/github/models/repository.py(4 hunks)backend/apps/github/utils.py(1 hunks)backend/pyproject.toml(0 hunks)
💤 Files with no reviewable changes (1)
- backend/pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
🔇 Additional comments (13)
backend/apps/github/utils.py (1)
36-43: LGTM! Clean refactoring to support multiple targets.The refactoring elegantly handles both single and multiple funding targets while maintaining backward compatibility. The early return pattern for non-compliant targets is efficient.
backend/apps/ai/management/commands/ai_create_chapter_chunks.py (2)
104-111: LGTM! Clean consolidation of prose field processing.The loop-based approach reduces code duplication and makes it easier to add new prose fields in the future.
125-136: Excellent refactoring of location information processing.The list comprehension with conditional filtering is elegant and the consolidation into a single "Location Information" entry improves readability of the output.
backend/apps/github/common.py (1)
56-63: LGTM! Excellent delegation to repository methods.The refactoring improves separation of concerns by moving synchronization logic to the Repository model where it belongs. The conditional check for archived repositories is preserved correctly.
backend/apps/ai/management/commands/ai_create_project_chunks.py (3)
94-102: Clever use of target list parameter to reduce branching.The approach of including the target list in the tuple elegantly determines where to append each field without additional conditionals.
125-137: LGTM! Clean statistics aggregation.The list comprehension with filtering for positive counts is efficient and the single "Project Statistics" entry improves output organization.
157-161: Well-structured helper method.The
_add_list_metadatahelper properly encapsulates the repeated logic for processing list-based fields.backend/apps/github/models/repository.py (6)
224-280: Excellent refactoring of the from_github method.The use of field mapping dictionary and optional data processors significantly reduces branching and improves maintainability. The code is now more modular and easier to extend.
281-288: Good error handling for empty repositories.The specific handling of the 409 status for empty Git repositories is appropriate.
314-319: Clean implementation of funding compliance check.The simplified approach using the updated
check_funding_policy_compliancefunction is much cleaner than nested loops.
454-465: LGTM! Good extraction of common logic.The
_update_assignees_and_labelshelper method effectively reduces code duplication between issue and pull request synchronization.
467-485: Smart optimization in release synchronization.The use of existing node IDs to avoid re-processing releases is an efficient approach. The early break when encountering an existing release assumes releases are ordered chronologically, which appears to be the case.
487-495: No changes needed for None handling insync_contributors
The comprehension’sif (user := User.update_data(…))clause already filters out anyNonereturns fromUser.update_data, so only validUserinstances reachbulk_save. No additional handling is required.Likely an incorrect or invalid review comment.
trucodd
changed the title
Proposed change
Resolves #1826
This PR addresses PLR0912 issue by:
Fixing 4 existing cases
Removing PLR0912 from pyproject.toml ignore list
Checklist
make check-testlocally; all checks and tests passed.