Add docstrings to mixin property methods#3532
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughDocstrings were added or expanded across GitHub repository/user mixins and the OWASP Project model; indexing eligibility logic was tightened in repository and user mixins, and Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Fix all issues with AI agents
In `@backend/apps/github/models/mixins/repository.py`:
- Around line 184-192: The property idx_subscribers_count currently returns the
wrong attribute (self.stars_count); update the property implementation for
idx_subscribers_count to return self.subscribers_count instead so it matches the
docstring and intended semantics; locate the idx_subscribers_count property in
the repository mixin and replace the returned symbol stars_count with
subscribers_count.
🧹 Nitpick comments (1)
backend/apps/github/models/mixins/user.py (1)
181-208: Addrepository__ownertoselect_related()to avoid N+1 queries on owner access.The query uses
.select_related("repository"), but accessingrc.repository.owner.loginat line 195 causes an additional query per iteration sinceowneris a ForeignKey that isn't included in the select_related.♻️ Suggested improvement
for rc in RepositoryContributor.objects.filter( user=self, ) .exclude( Q(repository__is_fork=True) | Q(repository__organization__is_owasp_related_organization=False) | Q(user__login__in=self.get_non_indexable_logins()) ) .order_by("-contributions_count") - .select_related("repository")[:TOP_REPOSITORY_CONTRIBUTORS_LIMIT] + .select_related("repository", "repository__owner")[:TOP_REPOSITORY_CONTRIBUTORS_LIMIT]Note: The access to
rc.repository.latest_release(lines 191-193) will also trigger queries sincelatest_releaseis a property method that cannot be optimized with prefetch or select_related mechanisms.
|
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
This is good, but any AI work needs to be reviewed by a PR author.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/github/models/mixins/user.py (1)
168-204: Potential N+1 queries inidx_contributionsforowneraccess;latest_releaserequires different approach.Line 185–189 accesses
rc.repository.owner(a ForeignKey), but the queryset onlyselect_related("repository"). This will trigger one extra query per row.Additionally, line 187 calls
rc.repository.latest_release.summary, which is a@propertythat executes.releases.filter(...).order_by(...).first()for each row—another N+1 concern. Sincelatest_releaseis not a database field but a computed property, it cannot be resolved withselect_related. Consider usingprefetch_related("repository__releases")or restructuring to fetch and filter releases more efficiently.At minimum, add
select_related("repository__owner"):♻️ Possible partial fix (owner only)
.select_related( "repository", + "repository__owner", )[:TOP_REPOSITORY_CONTRIBUTORS_LIMIT]For
latest_release, evaluate whetherprefetch_related("repository__releases")with post-processing, or a custom Prefetch object with filtered querysets, is more appropriate for your use case.
🧹 Nitpick comments (2)
backend/apps/owasp/models/project.py (2)
404-423: Property name doesn't match implementation (pre-existing).The docstring accurately describes the implementation, but note that the property is named
recent_milestoneswhile the query has no date/time filtering—it returns all milestones for the project's repositories. This is a pre-existing naming issue, not introduced by this PR.Consider renaming to
milestonesor adding date filtering in a follow-up if "recent" filtering is intended.
232-254: Consider adding type annotations for consistency.The
published_releasesandrecent_milestonesproperties now have return type annotations (models.QuerySet[Release]andmodels.QuerySet[Milestone]), but similar QuerySet-returning properties likeissues,open_issues, andpull_requestsdo not.For consistency, consider adding type annotations to these properties as well in a follow-up.
Example for `issues`
`@property` -def issues(self): +def issues(self) -> models.QuerySet[Issue]: """Get issues across the project's repositories.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3532 +/- ##
=======================================
Coverage 85.48% 85.48%
=======================================
Files 461 461
Lines 14240 14240
Branches 1896 1896
=======================================
Hits 12173 12173
Misses 1688 1688
Partials 379 379
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
2 participants
Proposed change
Resolves #2648
Files Updated:
• backend/apps/github/models/mixins/repository.py
• backend/apps/github/models/mixins/user.py
• backend/apps/owasp/models/mixins/project.py
What was changed:
• Added docstrings to all @Property methods in the mixin classes.
• Docstrings now explain:
• Purpose of the property (why it exists / how it is used).
• Return value and its type.
• Followed the existing docstring style in the codebase (verified from files like project health metrics, repository index, etc.).
Tests / Verification:
• Ran make check-test locally.
Checklist
make check-testlocally: all warnings addressed, tests passed