Extract magic numbers to constants in project.py (#2649) #2652
Conversation
Summary by CodeRabbit
WalkthroughExtracts hard-coded numeric literals into module-level constants and strengthens type annotations: adds MAX_HEALTH_SCORE, SOURCE_MAX_LENGTH, commit-related length constants; updates model field lengths and idx_health_score behavior; and adds/adjusts type hints in common utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 Ruff (0.14.4)backend/apps/common/utils.py15-15: Import from Import from (UP035) 16-16: Move third-party import Move into type-checking block (TC002) 🔇 Additional comments (9)
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/owasp/models/mixins/project.py (1)
14-14: Well done extracting the magic number!The constant name is descriptive and follows the established naming convention. One minor consideration: since
idx_health_scorereturnsfloat | None, you might consider defining this asHEALTH_SCORE_MAX = 100.0for strict type consistency, though Python's numeric type coercion makes this a low-priority refinement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/models/mixins/project.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/models/mixins/project.py (1)
backend/apps/owasp/models/project.py (1)
health_score(139-141)
🔇 Additional comments (1)
backend/apps/owasp/models/mixins/project.py (1)
41-44: LGTM! Magic number successfully replaced.The refactor correctly uses the named constant while preserving the existing logic. The TODO comment appropriately documents that returning the maximum score in production is temporary behavior.
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Hi, I found some more magic numbers
- at
backend/apps/github/models/mixins/release.pyline40 - not mixins, but you might check them as well:
backend/apps/github/models/commit.pylines62and63
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/ai/models/context.py(2 hunks)backend/apps/common/utils.py(7 hunks)backend/apps/github/models/commit.py(3 hunks)backend/apps/owasp/models/mixins/project.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/models/mixins/project.py
🧰 Additional context used
🪛 Ruff (0.14.4)
backend/apps/common/utils.py
14-14: Import from collections.abc instead: Iterable
Import from collections.abc
(UP035)
15-15: Move third-party import django.http.HttpRequest into a type-checking block
Move into type-checking block
(TC002)
🔇 Additional comments (6)
backend/apps/ai/models/context.py (2)
13-13: LGTM! Well-named constant.The constant name is descriptive and follows Python naming conventions for module-level constants.
23-24: LGTM! Magic number successfully extracted.The hardcoded
100has been correctly replaced with theSOURCE_MAX_LENGTHconstant, improving code maintainability while preserving the original behavior.backend/apps/github/models/commit.py (3)
34-38: LGTM! Field definition updated correctly.The
max_lengthparameter now uses theMAX_SHA_LENGTHconstant, successfully extracting the magic number while preserving the field's behavior.
64-68: LGTM! Magic numbers successfully extracted from str method.The hardcoded slice indices have been correctly replaced with named constants (
SHORT_SHA_LENGTHandMESSAGE_PREVIEW_LENGTH), making the code more maintainable and self-documenting.
10-12: <!-- [scratchpad_start]
[task review_file_2/3 review_segment_1/3 verification_sha_length - COMPLETED]
Final verification of MAX_SHA_LENGTH = 64
[observations]
- Git commit SHA: SHA-1 = 160-bit (20-byte) hash = 40-character hexadecimal string
- All sources confirm: full Git SHA-1 is 40 characters
- GitHub API returns 40-character SHA-1 hashes for commits (confirmed in search result 15-12, 18-35)
- Short SHA (displayed by Git and GitHub): 7 characters (confirmed by multiple sources)
- SHORT_SHA_LENGTH = 7 in the code is correct
- MAX_SHA_LENGTH = 64 in the code exceeds the 40-character SHA-1 standard
- 64 characters matches SHA-256 specification (future preparation)
[analysis]
FINDING: The MAX_SHA_LENGTH = 64 does NOT align with GitHub API current behavior- GitHub API returns SHA-1 hashes (40 characters max)
- Setting field to 64 is over-provisioned
- This appears intentional (preparing for SHA-256 migration) but not aligned with current reality
- The original review comment asked to "verify that the database field and GitHub API responses align with the 64-character limit"
- VERDICT: They do NOT align - GitHub API returns 40-char SHAs, not 64-char
The original review was actually requesting verification that contradicts what the code does.
The code is not "wrong" per se - it's conservative/forward-compatible - but the original verification request was mis-stated.DECISION: This is a case where the original review comment's concern is technically valid but not critical:
- The constants are well-defined
- The database schema is consistent
- The field can store GitHub API responses (40 chars < 64 chars capacity)
- The extra capacity is unusual but not problematic
Should tag this as because while verification confirms the mismatch noted in the original request, this is not a functional issue. The database will work correctly with current GitHub API.
Actually, re-reading the original comment more carefully, it says "If
MAX_SHA_LENGTH = 64was the original value, this refactor correctly preserves the existing behavior" with a tag of . This means the original reviewer was actually approving the code IF it was preserving existing behavior.But the original also asks to "Verify that the database field and GitHub API responses align with the 64-character limit" - which they don't. The API returns 40-char values, not 64-char.
This is a borderline case. The constants are correct and don't need fixing. The verification found they're misaligned with GitHub API spec, but not in a way that breaks the code.
Final judgment: - flag for developer to confirm this over-provisioning is intentional
[done]
</scratchpad_start] -->Verify the reasoning behind MAX_SHA_LENGTH = 64.
GitHub API returns full, 40-character SHA-1 hashes for commits, but your code sets
MAX_SHA_LENGTH = 64. Git's SHA-1 hash algorithm generates a 160-bit (20-byte) hash value, commonly expressed as a 40-character hexadecimal number. The 64-character capacity appears to prepare for SHA-256 migration rather than match current GitHub API responses.The constant definitions are well-formed and consistent with the database schema. However, confirm with your team whether intentionally over-provisioning the SHA field is acceptable, or if it should be set to 40 to match GitHub's specification.
backend/apps/common/utils.py (1)
52-52: Type hint improvements look good but seem out of scope.The type hint additions (
Optional[str],HttpRequest,Iterable[Any],int | str | datetime,Optional[str]) improve type safety and code documentation. However, these changes don't align with the PR's stated objective of extracting magic numbers. Consider whether these type hint improvements should be in a separate PR focused on type safety enhancements.Can you clarify whether these type hint changes were intentionally included in this PR, or if they should be part of a separate type-safety focused PR?
Also applies to: 91-91, 108-108, 122-122, 142-142, 199-199
| from typing import Optional, Iterable, Any | ||
| from django.http import HttpRequest | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Address static analysis hints for import optimization.
The static analysis tools have identified two valid improvements:
Iterableshould be imported fromcollections.abcinstead oftyping(deprecated as of Python 3.9)HttpRequestis only used in type annotations and should be moved into aTYPE_CHECKINGblock to avoid runtime import overhead
Apply this diff to fix the imports:
-from typing import Optional, Iterable, Any
-from django.http import HttpRequest
-
+from collections.abc import Iterable
+from typing import TYPE_CHECKING, Any, Optional
+
+if TYPE_CHECKING:
+ from django.http import HttpRequestAs per static analysis hints.
📝 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.
| from typing import Optional, Iterable, Any | |
| from django.http import HttpRequest | |
| from collections.abc import Iterable | |
| from typing import TYPE_CHECKING, Any, Optional | |
| if TYPE_CHECKING: | |
| from django.http import HttpRequest |
🧰 Tools
🪛 Ruff (0.14.4)
14-14: Import from collections.abc instead: Iterable
Import from collections.abc
(UP035)
15-15: Move third-party import django.http.HttpRequest into a type-checking block
Move into type-checking block
(TC002)
🤖 Prompt for AI Agents
In backend/apps/common/utils.py around lines 14 to 16, update the imports to
import Iterable from collections.abc and move HttpRequest into a TYPE_CHECKING
block: replace "from typing import Optional, Iterable, Any" with "from typing
import Optional, Any" and add "from collections.abc import Iterable", then wrap
"from django.http import HttpRequest" inside "if TYPE_CHECKING:" after adding
"from typing import TYPE_CHECKING" at the top so HttpRequest is only imported
for type checking and avoids runtime overhead.
|
|
Hi @sonu1680, it seems you've updated the PR. Feel free to ping me or request a review when you think it's good to go. |
2 participants
Proposed change
Resolves #2649
This pull request extracts hardcoded numeric values (magic numbers) from the
project.pymixin into descriptive module-level constants.The goal is to improve readability, maintainability, and consistency within the
codebase by avoiding unnamed literal values inside methods.
What was changed
backend/apps/owasp/models/mixins/project.pymake check-testChecklist
make check-testlocally; all checks and tests passed.