Enhance and add docstrings to sitemap views, following style guide from base.py and repository.py #2669
Conversation
…om base.py and repository.py
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds return type annotations and expanded docstrings across sitemap view classes; several methods also change how items are produced (ordering, source queryset vs. comprehension) affecting Member, Organization, Repository, and Snapshot sitemaps. Tests updated to match new query/order behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (1)
backend/apps/sitemap/views/organization.py (1)
14-19: Consider minor wording adjustment for consistency.The one-line summary on line 14 says "queryset" but the method returns a list (list comprehension), not a QuerySet. The Returns section correctly documents it as "list". Consider updating line 14 to:
- """Return a queryset of indexable Organization objects. + """Return a list of indexable Organization objects for sitemap generation.This would align the summary with the actual return type and match the pattern used in other similar files (chapter.py, committee.py, project.py).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/sitemap/views/chapter.py(1 hunks)backend/apps/sitemap/views/committee.py(1 hunks)backend/apps/sitemap/views/member.py(1 hunks)backend/apps/sitemap/views/organization.py(1 hunks)backend/apps/sitemap/views/project.py(1 hunks)backend/apps/sitemap/views/repository.py(1 hunks)backend/apps/sitemap/views/snapshot.py(2 hunks)backend/apps/sitemap/views/static.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/sitemap/views/static.py (1)
backend/apps/sitemap/views/base.py (4)
location(46-48)items(38-40)BaseSitemap(7-52)lastmod(42-44)
🔇 Additional comments (13)
backend/apps/sitemap/views/project.py (1)
14-19: LGTM! Clear and accurate docstring.The docstring accurately describes the return value and ordering behavior. The documentation clearly states that it returns a list of indexable Project objects, which matches the list comprehension implementation.
backend/apps/sitemap/views/committee.py (1)
14-19: LGTM! Documentation is clear and accurate.The enhanced docstring properly documents the return type and ordering, matching the implementation.
backend/apps/sitemap/views/chapter.py (1)
14-19: LGTM! Well-documented method.The docstring enhancement accurately describes the method's behavior and return value.
backend/apps/sitemap/views/repository.py (2)
16-21: LGTM! Comprehensive and accurate docstring.The docstring accurately documents the QuerySet return type and clearly describes the filtering criteria (non-archived, non-empty, non-template repositories with organizations).
33-43: LGTM! Clear parameter and return documentation.The Args and Returns sections provide clear documentation for the location method.
backend/apps/sitemap/views/member.py (1)
14-18: LGTM! Clear and accurate documentation.The docstring accurately describes the return value, including the important detail about excluding bots.
backend/apps/sitemap/views/static.py (5)
19-29: LGTM! Clear documentation for changefreq method.The docstring properly documents the parameter and return value.
31-41: LGTM! Clear documentation for location method.The Args and Returns sections are clear and accurate.
43-50: LGTM! Accurate documentation for items method.The docstring correctly describes the return value as a tuple of static route configurations from BaseSitemap.STATIC_ROUTES.
52-77: LGTM! Comprehensive documentation for lastmod method.The docstring accurately describes the complex logic of mapping paths to models and returning the latest update timestamp or current time as fallback. The detailed Returns section clearly explains this behavior.
79-89: LGTM! Clear documentation for priority method.The docstring properly documents the parameter and return type with appropriate range information (0.0 to 1.0).
backend/apps/sitemap/views/snapshot.py (2)
14-19: LGTM! Accurate and clear docstring.The docstring correctly documents the QuerySet return type and clearly describes the filtering criteria (completed snapshots ordered by update/creation date).
27-36: LGTM! Well-documented location method.The Args and Returns sections provide clear documentation for the method's parameter and return value.
There was a problem hiding this comment.
This is not right. Please create a feature branch and make your changes in this branch not the main. Please see contributing guidelines
@ahmedxgouda it should be fine, yes it's a good idea to have a separate branch, but this PR can be reviewed as is. It's their fork and it doesn't really affect our code base, right? |
Yes, I'll take care to make changes through a seperate branch from next time. |
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Looks good to me except for this minor suggestion.
| """Return list of projects for sitemap generation. | ||
| Returns: | ||
| list: List of indexable Project objects ordered by update/creation date. |
There was a problem hiding this comment.
| list: List of indexable Project objects ordered by update/creation date. | |
| list: List of indexable active Project objects ordered by update/creation date. |
There was a problem hiding this comment.
Hi, updated as suggested. Thanks!
|
|
||
| def items(self) -> QuerySet[Repository]: | ||
| """Return list of repositories for sitemap generation.""" | ||
| """Return list of repositories for sitemap generation. |
There was a problem hiding this comment.
It doesn't look like a list.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/apps/sitemap/views/repository.py (1)
13-28: Docstring and return type look good; minor consistency nit
items()now clearly returns alist[Repository]and the docstring accurately describes the ordering and indexability filter. To keep the docstrings consistent with other sitemap views (e.g., Chapter/Project usinglist:in theReturns:section), you might consider usinglist:here as well or updating the others to thelist[Repository]-style for uniformity.backend/tests/apps/sitemap/views/repository_test.py (1)
20-29: Mock wiring matches newitems()implementationThe test now correctly mocks
Repository.objects.order_byto return an iterable that yieldsmock_obj, which aligns with the updateditems()implementation. Wrappingsitemap.items()inlist()is redundant now thatitems()already returns a list, but it’s harmless and doesn’t affect correctness.backend/apps/sitemap/views/member.py (1)
13-29: Member sitemapitems()matches its docstring; tiny style tweak possibleThe implementation correctly returns indexable, non-bot users ordered by update/creation date, and the type hint/docstring describe that accurately. For consistency with the other sitemap docstrings, you could add a trailing period to the “Returns:” description line (
... update/creation date.), but that’s purely cosmetic.backend/apps/sitemap/views/static.py (1)
19-28: Docstring and type‑hint updates are consistent with static route structureThe new docstrings and return annotations for
changefreq,location,items, andprioritycorrectly describe the actual behavior and the shape ofBaseSitemap.STATIC_ROUTES. One small optional improvement would be to type theitemparameter consistently across these methods (e.g.,item: dictoritem: Mapping[str, Any]) to match the docstrings that already describe it as a dictionary.Also applies to: 31-40, 43-49, 79-88
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/apps/sitemap/views/chapter.py(1 hunks)backend/apps/sitemap/views/committee.py(1 hunks)backend/apps/sitemap/views/member.py(1 hunks)backend/apps/sitemap/views/organization.py(1 hunks)backend/apps/sitemap/views/project.py(1 hunks)backend/apps/sitemap/views/repository.py(1 hunks)backend/apps/sitemap/views/snapshot.py(3 hunks)backend/apps/sitemap/views/static.py(2 hunks)backend/tests/apps/sitemap/views/member_test.py(1 hunks)backend/tests/apps/sitemap/views/organization_test.py(1 hunks)backend/tests/apps/sitemap/views/repository_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/sitemap/views/snapshot.py
- backend/apps/sitemap/views/committee.py
🧰 Additional context used
🧬 Code graph analysis (7)
backend/apps/sitemap/views/organization.py (7)
backend/apps/sitemap/views/chapter.py (1)
items(13-27)backend/apps/sitemap/views/committee.py (1)
items(13-27)backend/apps/sitemap/views/member.py (1)
items(13-29)backend/apps/sitemap/views/project.py (1)
items(13-27)backend/apps/sitemap/views/repository.py (1)
items(13-28)backend/apps/sitemap/views/snapshot.py (1)
items(15-27)backend/apps/sitemap/views/base.py (1)
items(38-40)
backend/tests/apps/sitemap/views/organization_test.py (1)
backend/tests/apps/github/api/internal/queries/organization_test.py (1)
mock_organization(15-20)
backend/apps/sitemap/views/chapter.py (3)
backend/apps/sitemap/views/committee.py (1)
items(13-27)backend/apps/sitemap/views/base.py (1)
items(38-40)backend/apps/owasp/models/chapter.py (1)
Chapter(21-217)
backend/apps/sitemap/views/static.py (1)
backend/apps/sitemap/views/base.py (6)
changefreq(34-36)location(46-48)items(38-40)BaseSitemap(7-52)lastmod(42-44)priority(50-52)
backend/apps/sitemap/views/repository.py (6)
backend/apps/sitemap/views/chapter.py (1)
items(13-27)backend/apps/sitemap/views/committee.py (1)
items(13-27)backend/apps/sitemap/views/member.py (1)
items(13-29)backend/apps/sitemap/views/organization.py (1)
items(13-28)backend/apps/sitemap/views/project.py (1)
items(13-27)backend/apps/sitemap/views/base.py (1)
items(38-40)
backend/apps/sitemap/views/member.py (9)
backend/apps/sitemap/views/chapter.py (1)
items(13-27)backend/apps/sitemap/views/committee.py (1)
items(13-27)backend/apps/sitemap/views/organization.py (1)
items(13-28)backend/apps/sitemap/views/project.py (1)
items(13-27)backend/apps/sitemap/views/repository.py (1)
items(13-28)backend/apps/sitemap/views/snapshot.py (1)
items(15-27)backend/apps/sitemap/views/static.py (1)
items(43-50)backend/apps/sitemap/views/base.py (1)
items(38-40)backend/apps/github/models/user.py (1)
User(28-216)
backend/apps/sitemap/views/project.py (9)
backend/apps/sitemap/views/chapter.py (1)
items(13-27)backend/apps/sitemap/views/committee.py (1)
items(13-27)backend/apps/sitemap/views/member.py (1)
items(13-29)backend/apps/sitemap/views/organization.py (1)
items(13-28)backend/apps/sitemap/views/repository.py (1)
items(13-28)backend/apps/sitemap/views/snapshot.py (1)
items(15-27)backend/apps/sitemap/views/static.py (1)
items(43-50)backend/apps/sitemap/views/base.py (1)
items(38-40)backend/apps/owasp/models/project.py (1)
Project(37-384)
🔇 Additional comments (5)
backend/tests/apps/sitemap/views/member_test.py (1)
16-23: Chained queryset mock correctly mirrorsitems()queryConfiguring
mock_user.objects.filter.return_value.order_by.return_value = [mock_obj]matches the production chain and makesitems()iterate over the expected user. The assertion onlist(sitemap.items())is straightforward and aligns with the new implementation.backend/tests/apps/sitemap/views/organization_test.py (1)
16-24: Updated mock reflectsrelated_organizations.order_byusageThe test’s use of
mock_organization.related_organizations.order_by.return_value = mock_querysetplus a custom__iter__onmock_querysetcorrectly simulates the newitems()implementation. This keeps the test aligned with how organizations are sourced and ordered for the sitemap.backend/apps/sitemap/views/chapter.py (1)
13-27: Chapteritems()typing and docstring are consistentThe
items()method’s type annotation, implementation (list comprehension overactive_chapters.order_bywithis_indexablefilter), and docstring all line up and mirror the style used in other sitemap views.backend/apps/sitemap/views/project.py (1)
13-27: Projectitems()implementation and docstring align
items()correctly advertises and returns a list of indexable projects ordered byupdated_at/created_at, consistent with the active_projects manager and with the docstring/annotation.backend/apps/sitemap/views/organization.py (1)
13-28: Organization sitemapitems()is well-documented and consistentThe method sources organizations via
related_organizations.order_by(...)and filters onis_indexable, which matches the “OWASP-related” wording in the docstring. The return type annotation andReturns:section are in sync with the actual list comprehension.
3 participants
Proposed change
Resolves #2645
Changes made -
Enhanced docstrings across 8 files in the sitemap views:
chapter.pycommittee.pymember.pyorganization.pyproject.pyrepository.pysnapshot.pystatic.pyAll the docstrings now include detailed
ArgsandReturnssections where appropriate, following the style guide from base.py and repository.py .Checklist
make check-testlocally; all checks and tests passed.