Fixed naming convention error for totalCount #2901
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR implements a systematic rename of the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/github/models/repository_test.py (1)
125-129: Test mock should match PyGithub's actual API.The mock uses
total_count, but PyGithub's actual PaginatedList hastotalCount. While tests can mock any attribute, this creates a discrepancy between test and production behavior.For consistency with PyGithub's API, consider:
- type(commits_mock).total_count = PropertyMock( + type(commits_mock).totalCount = PropertyMock( side_effect=GithubException( status=409, data={"message": "Git Repository is empty", "status": "409"} ) )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/github/management/commands/github_sync_user.py(9 hunks)backend/apps/github/management/commands/github_update_owasp_organization.py(1 hunks)backend/apps/github/management/commands/github_update_related_organizations.py(1 hunks)backend/apps/github/models/repository.py(1 hunks)backend/tests/apps/github/management/commands/github_sync_user_test.py(1 hunks)backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py(3 hunks)backend/tests/apps/github/management/commands/github_update_related_organizations_test.py(1 hunks)backend/tests/apps/github/models/repository_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/github/models/repository.py (1)
backend/apps/owasp/api/internal/nodes/committee.py (1)
contributors_count(15-17)
| direction="desc", | ||
| ) | ||
| gh_repositories_count = gh_repositories.totalCount # type: ignore[attr-defined] | ||
| gh_repositories_count = gh_repositories.total_count # type: ignore[attr-defined] |
There was a problem hiding this comment.
Critical: The # type: ignore[attr-defined] comment confirms this attribute doesn't exist.
The type: ignore suppresses the type checker but does not prevent the runtime AttributeError. PyGithub's PaginatedList only exposes totalCount (camelCase), not total_count.
Apply this diff:
- gh_repositories_count = gh_repositories.total_count # type: ignore[attr-defined]
+ gh_repositories_count = gh_repositories.totalCount📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gh_repositories_count = gh_repositories.total_count # type: ignore[attr-defined] | |
| gh_repositories_count = gh_repositories.totalCount |
🤖 Prompt for AI Agents
In backend/apps/github/management/commands/github_update_owasp_organization.py
around line 70, replace the incorrect use of gh_repositories.total_count (which
doesn't exist at runtime) with the correct PyGithub attribute
gh_repositories.totalCount; remove the `# type: ignore[attr-defined]` and
optionally add a safe fallback (e.g., use getattr(gh_repositories, "totalCount",
0) or wrap access in try/except AttributeError) so the code won't raise at
runtime if the attribute is missing.
backend/apps/github/management/commands/github_update_related_organizations.py
Show resolved
Hide resolved
| self.total_count = len(items) #for linting errors | ||
| self.totalCount = len(items) #for PyGithub compatibility |
There was a problem hiding this comment.
Dual-attribute approach in mocks doesn't fix the production runtime errors.
While this mock provides both total_count (line 19) and totalCount (line 20), the production code still accesses .total_count on real PyGithub PaginatedList objects, which only expose totalCount. This makes tests pass but production will fail.
The correct solution: Production code should use totalCount (with linting suppressions), and mocks should only provide totalCount to match PyGithub's actual behavior.
Apply this diff to match PyGithub's API:
def __init__(self, items):
self._items = items
- self.total_count = len(items) #for linting errors
- self.totalCount = len(items) #for PyGithub compatibility
+ self.totalCount = len(items) # PyGithub's PaginatedList attribute name🤖 Prompt for AI Agents
In backend/tests/apps/github/management/commands/github_sync_user_test.py around
lines 19-20, the mock incorrectly defines both total_count and totalCount;
update the mock to only expose totalCount (remove the total_count attribute) so
it matches PyGithub's PaginatedList, and update production code to consistently
access .totalCount (add a lint suppression comment where needed) so runtime
behavior aligns with the real API.
| class PaginatedListMock(list): | ||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.totalCount = len(self) | ||
| self.total_count = len(self) | ||
|
|
||
| def __getitem__(self, index): | ||
| if isinstance(index, slice): | ||
| result = super().__getitem__(index) | ||
| new_list = PaginatedListMock(result) | ||
| new_list.totalCount = self.totalCount | ||
| new_list.total_count = self.total_count | ||
| return new_list | ||
| return super().__getitem__(index) | ||
|
|
There was a problem hiding this comment.
PaginatedListMock should use totalCount to match PyGithub's API.
The mock class uses total_count, but PyGithub's actual PaginatedList provides totalCount (camelCase). This creates inconsistency between tests and production behavior.
Apply this diff to all occurrences:
class PaginatedListMock(list):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
- self.total_count = len(self)
+ self.totalCount = len(self)
def __getitem__(self, index):
if isinstance(index, slice):
result = super().__getitem__(index)
new_list = PaginatedListMock(result)
- new_list.total_count = self.total_count
+ new_list.totalCount = self.totalCount
return new_list
return super().__getitem__(index)And update line 218:
- mock_repos.total_count = 3
+ mock_repos.totalCount = 3Also applies to: 198-210, 218-218
🤖 Prompt for AI Agents
In
backend/tests/apps/github/management/commands/github_update_owasp_organization_test.py
around lines 108-120 (also apply same change at 198-210 and update line 218),
the PaginatedListMock uses total_count but PyGithub uses camelCase totalCount;
change the attribute name and any references from total_count to totalCount,
ensure the __init__ sets self.totalCount = len(self), and when slicing create
the new PaginatedListMock and copy self.totalCount to new_list.totalCount, and
update line 218 to reference totalCount as well so the mock matches PyGithub
behavior.
| gh_org = mock.Mock() | ||
| gh_repos = mock.MagicMock() | ||
| gh_repos.totalCount = num_repos | ||
| gh_repos.total_count = num_repos |
There was a problem hiding this comment.
Test mock should use totalCount to match PyGithub's API.
PyGithub's PaginatedList exposes totalCount (camelCase), not total_count. The mock should match the actual library interface.
Apply this diff:
- gh_repos.total_count = num_repos
+ gh_repos.totalCount = num_repos📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gh_repos.total_count = num_repos | |
| gh_repos.totalCount = num_repos |
🤖 Prompt for AI Agents
In
backend/tests/apps/github/management/commands/github_update_related_organizations_test.py
around line 53, the test mock sets gh_repos.total_count but PyGithub's
PaginatedList uses totalCount (camelCase); change the mock to set
gh_repos.totalCount = num_repos (and remove or stop using total_count) so the
test matches the actual PyGithub interface and behavior.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anirudhprmar Please address all CodeRabbit comments
yeah, on it |
|
@kasya, the changes coderabbit suggests revert back to the original implementation, which makes this entire issue irrelevant. Also i noticed that changing totalCount to total_count is creating more problems and errors. This time i even tried to implement use getattr(commits, 'totalCount', 0) but this still fails the make check-tests I don't think this issue is a good fix. |
|
2 participants
Proposed change
Resolves #2887
The problem was totalCount didn't matched the required regular expression ^[_a-z][_a-z0-9]*$, meaning it wasn't snake_case as python supports that. When trying to solve for this issue, i renamed all the files using totalCount as field name but when i ran make check-test, i was getting some additional unnecessary error. I went deep and found out that PyGithub utilization in repository.test file was giving run time errors as according to docs PyGithub intentionally uses camelCase for totalCount to match GitHub's JSON response patterns, even though it breaks Python conventions.
To solve this there were only 2 ways:
So, I implemented the first solution. Willing to hear suggestions on what should be done, keep it as it as or go with getattr().
Checklist
make check-testlocally and all tests passed