refactor:remove PLR0912 ignore and fix 4 violations #1834
Conversation
Summary by CodeRabbit
WalkthroughThe codebase was refactored to address and eliminate violations of the "too-many-branches" (PLR0912) linter rule. Several large functions were modularized by extracting logic into helper methods, reducing branching complexity. The Ruff linter configuration was updated to remove the PLR0912 rule from the ignore list, enabling enforcement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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 (1)
backend/apps/github/models/repository.py (1)
298-303: Consider improving readability of the funding compliance check.While the comprehension is concise, the nested logic with conditional list handling might be difficult to understand. Consider extracting this to a separate method or using a more explicit loop for better maintainability.
-self.is_funding_policy_compliant = all( - check_funding_policy_compliance(platform, target) - for platform, targets in self.funding_yml.items() - for target in (targets if isinstance(targets, list) else [targets]) - if target -) +self.is_funding_policy_compliant = self._check_all_funding_compliance()Add this helper method:
def _check_all_funding_compliance(self) -> bool: """Check if all funding targets are policy compliant.""" for platform, targets in self.funding_yml.items(): # Normalize to list for consistent processing target_list = targets if isinstance(targets, list) else [targets] for target in target_list: if target and not check_funding_policy_compliance(platform, target): return False return True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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(2 hunks)backend/apps/github/models/repository.py(2 hunks)backend/pyproject.toml(0 hunks)
💤 Files with no reviewable changes (1)
- backend/pyproject.toml
🔇 Additional comments (14)
backend/apps/ai/management/commands/ai_create_chapter_chunks.py (4)
104-108: Good refactoring to reduce code duplication.The use of
getattrwith a loop effectively reduces repetitive conditional blocks while maintaining the same functionality.
110-116: Safe repository access with getattr.Good use of
getattrto safely access theowasp_repositoryattribute, preventing potential AttributeError.
121-132: Elegant consolidation of location fields.The list comprehension effectively consolidates multiple conditional checks into a single, readable operation while maintaining the same filtering logic.
135-146: Well-structured metadata processing.The loop-based approach with dynamic type handling for lists effectively reduces code duplication while preserving the original logic.
backend/apps/github/models/repository.py (2)
250-259: Excellent modularization pattern.The use of a loop with (data, processor) tuples elegantly handles optional data sources and reduces branching complexity.
270-277: Clean extraction of commits processing logic.The method properly handles the edge case of empty repositories with appropriate exception handling.
backend/apps/ai/management/commands/ai_create_project_chunks.py (4)
93-103: Clever use of target list in tuple pattern.The inclusion of the target list in the tuple allows for dynamic routing of content to the appropriate list, effectively reducing code duplication.
125-137: Efficient statistics aggregation.Good use of list comprehension with filtering to create a concise statistics summary, excluding zero values.
157-162: Clean helper method for list metadata.Simple and effective extraction that reduces code duplication for list-based metadata fields.
163-187: Well-organized additional metadata handling.The method effectively groups related metadata logic, maintaining clear separation of concerns.
backend/apps/github/common.py (4)
42-70: Excellent modularization of sync_repository.The refactoring transforms a complex function into a clear, high-level orchestrator that delegates specific sync operations to focused helper functions. This greatly improves code maintainability and readability.
72-98: Well-structured milestone synchronization.The extracted function maintains the original logic with proper date-based filtering and exception handling.
174-188: Excellent DRY implementation.The shared helper function effectively eliminates code duplication between issue and pull request processing while maintaining proper error handling.
212-221: Elegant use of walrus operator.The list comprehension with walrus operator efficiently handles user updates while filtering out None values in a single pass.
Hey I didnt make any new changes just marked for review again let me know your suggestions |
@trucodd I'm off for today, will review it later. Feel free to join OWASP Slack if you want to discuss anything it DM. |
trucodd
force-pushed
the
feature/fix-plr0912
branch
2 times, most recently
from
845a82c to
d6cdb73
Compare
trucodd
force-pushed
the
feature/fix-plr0912
branch
from
d6cdb73 to
80fc1e1
Compare
| if hasattr(chapter, "owasp_repository") and chapter.owasp_repository: | ||
| repo = chapter.owasp_repository | ||
| # Prose content | ||
| for field, label in [("description", "Description"), ("summary", "Summary")]: |
There was a problem hiding this comment.
Use tuples for immutable containers
backend/apps/github/common.py
Outdated
| _sync_repository_milestones(gh_repository, repository) | ||
| _sync_repository_issues(gh_repository, repository) | ||
| _sync_repository_pull_requests(gh_repository, repository) | ||
|
|
||
| _sync_repository_releases(gh_repository, repository, is_owasp_site_repository) | ||
| _sync_repository_contributors(gh_repository, repository) |
There was a problem hiding this comment.
It looks like these can be defined as Repository model methods, it's fine to make it fatter.
| # FKs. | ||
| self.organization = organization | ||
| self.owner = user | ||
| def _check_all_funding_compliance(self) -> bool: |
There was a problem hiding this comment.
Maybe it'd be better to refactor check_funding_policy_compliance function to accept an iterable instead of a single target?
|
Proposed change
Resolves #1826
This PR addresses PLR0912 issues by:
Fixing 4 existing cases
Removing PLR0912 from pyproject.toml ignore list
Checklist
make check-testlocally; all checks and tests passed.