Add type hints to utils.py files #2670
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded and refined type hints across multiple backend utility modules, made several parameters optional, adjusted a few return types (including returning empty string on certain failures), and updated docstrings to match the new signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/apps/slack/utils.py (2)
81-114: Fix docstring type mismatch for timeout parameter.The docstring at line 87 states
timeout (int, optional), but the actual parameter type isfloat | None. This inconsistency should be corrected.Apply this diff to fix the docstring:
@lru_cache def get_news_data(limit: int = 10, timeout: float | None = 30) -> list[dict[str, str]]: """Get news data. Args: limit (int, optional): The maximum number of news items to fetch. - timeout (int, optional): The request timeout in seconds. + timeout (float | None, optional): The request timeout in seconds. Returns: list: A list of dictionaries containing news data (author, title, and URL). """
118-141: Fix docstring type mismatch for timeout parameter.Similar to
get_news_data, the docstring at line 122 statestimeout (int, optional), but the actual parameter type isfloat | None.Apply this diff to fix the docstring:
@lru_cache def get_staff_data(timeout: float | None = 30) -> list[dict[str, Any]] | None: """Get staff data. Args: - timeout (int, optional): The request timeout in seconds. + timeout (float | None, optional): The request timeout in seconds. Returns: list or None: A sorted list of staff data dictionaries, or None if an error occurs. """
🧹 Nitpick comments (2)
backend/apps/github/utils.py (2)
60-82: Consider removing the else clause for consistency.The
elseclause afterexceptis valid but unnecessary here. The idiomatic pattern would be to return directly after the try block since the except already returns.Apply this diff for a more conventional structure:
def get_repository_file_content( url: str, *, timeout: float | None = 30, ) -> str: """Get the content of a file from a repository. Args: url (str): The URL of the file. timeout (float | None, optional): The request timeout in seconds. Returns: str: The content of the file, or empty string if the request fails. """ try: response: requests.Response = requests.get(url, timeout=timeout) except RequestException as e: logger.exception("Failed to fetch file", extra={"URL": url, "error": str(e)}) return "" - else: - return response.text + return response.text
114-121: Minor: Consider extracting prefix constants to module level.The
http_prefixandhttps_prefixvariables are constants used within this function. While local annotations are fine, these could be module-level constants for reusability and clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/utils.py(9 hunks)backend/apps/github/utils.py(5 hunks)backend/apps/slack/utils.py(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T12:57:58.021Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2223
File: backend/apps/owasp/models/common.py:0-0
Timestamp: 2025-09-06T12:57:58.021Z
Learning: When filtering URLs by domain in the OWASP project, use `urlparse(url).netloc == domain` instead of `domain in url` to avoid security vulnerabilities where malicious subdomains could bypass filtering (e.g., `testowasp.org` would match when filtering for `owasp.org`).
Applied to files:
backend/apps/github/utils.py
🔇 Additional comments (8)
backend/apps/github/utils.py (1)
36-57: LGTM! Proper security-conscious domain checking.The type hints are correct, and the use of
urlparse(target).netlocfor domain extraction follows security best practices to avoid subdomain bypasses.backend/apps/slack/utils.py (2)
25-35: LGTM! Type hint correctly added.The return type annotation is accurate and the function logic remains unchanged.
183-233: LGTM! Type hints properly added.The parameter type
tuple[dict[str, Any], ...]and local variabletext: list[str]annotations are accurate.backend/apps/common/utils.py (5)
17-19: LGTM! Proper use of TYPE_CHECKING.Using
TYPE_CHECKINGto conditionally importHttpRequestis the correct pattern to avoid circular imports while maintaining type safety.
21-38: LGTM! Local variable type annotations added correctly.The explicit type annotations for
parts,offset,head, andsegmentsimprove code clarity without changing logic.
143-160: LGTM! Type hints correctly widen the parameter type.The union type
int | str | datetimeproperly reflects the function's capability to handle multiple input types, and the localdt: datetimeannotation improves clarity.
220-238: LGTM! Type hints correctly handle None inputs.The parameter type
str | Noneproperly reflects that the function can acceptNoneand will returnFalsein that case.
93-109: Review comment is accurate; no issues found after verification.Based on verification of the codebase:
Intentional behavior change: The function signature declares
-> str(notstr | None), making the empty string return intentional and consistent with the type contract.Downstream handling is safe: The only downstream usage is in
backend/apps/core/api/internal/algolia.pywhereip_addressis:
- Interpolated into a cache key string (line 57) — empty string is harmless
- Passed to
get_search_results()and ultimatelyIndexBase.get_client(ip_address=None)— both accept optional parameters safelyMinor test coverage gap (not a blocker): Current tests cover the
HTTP_X_FORWARDED_FORandREMOTE_ADDRpresent cases, plus the local environment case, but lack an explicit test for both headers missing in a non-local environment. This is the scenario that triggers the empty string return. Adding a test case would improve coverage but is not critical since empty string is handled gracefully throughout the codebase.
|
hii @arkid15r can you plese review this PR |
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Hello, thank you for working on this.
Add complete type hints to all utility functions:
Parameter types
Return types
Optional parameters with proper None handling
You can remove the type annotations in the function code, and only modify it for parameters, return types, and docstrings.
Please use a consistent format for docstrings. I have left a few examples of what needs to be changed below:
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/slack/utils.py (1)
82-99: Minor docstring/type mismatch on timeout in get_news_dataThe signature uses
timeout: float | None = 30, but the docstring still documentstimeout (int, optional). Consider aligning the docstring with the annotation (e.g.,float | None) so the documented API matches the typed one.
♻️ Duplicate comments (1)
backend/apps/slack/utils.py (1)
163-172: get_posts_data return docstring still mentions sponsorsThe function retrieves posts (
Post.recent_posts()), but theReturns:line now says “A queryset of sponsors, or None if an error occurs.” This is misleading and matches a concern already raised in past review comments; it should refer to posts, not sponsors.Suggested fix:
- Returns: - QuerySet or None: A queryset of sponsors, or None if an error occurs. + Returns: + QuerySet or None: A queryset of posts, or None if an error occurs.
🧹 Nitpick comments (3)
backend/apps/slack/utils.py (3)
56-77: Tightened return type for get_gsoc_projects is good; consider updating docstring
get_gsoc_projects(year: int) -> list[dict[str, Any]]better reflects reality than a barelist. The docstringReturns:section still says justlist:; you may want to update it tolist[dict[str, Any]](or similar wording) for consistency with the new type hint.
118-141: get_staff_data typing is accurate; docstring could mirror types more closelyThe return type
list[dict[str, Any]] | Noneandtimeout: float | None = 30look reasonable and match the usage. As withget_news_data, the docstring still saystimeout (int, optional)andlist or None:; if you want full consistency, you could update these to reflectfloat | Noneandlist[dict[str, Any]] | None.
183-194: get_text blocks type is correct but could be generalizedAnnotating
blocksastuple[dict[str, Any], ...]andtextaslist[str]is type-safe for current usage. However, Slack blocks are commonly passed around as lists; if call sites use lists instead of tuples, a more general type such asSequence[dict[str, Any]](orIterable[dict[str, Any]]) would better match real usage and avoid static-type noise.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/utils.py(9 hunks)backend/apps/github/utils.py(5 hunks)backend/apps/slack/utils.py(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/github/utils.py
- backend/apps/common/utils.py
🔇 Additional comments (4)
backend/apps/slack/utils.py (4)
9-14: Typing imports and TYPE_CHECKING block look correctUsing
TYPE_CHECKINGforQuerySetand importingAnyis consistent with typical Django typing patterns; no issues here.
25-35: escape() annotation aligns with implementation
escape(content: str) -> strcorrectly matches theescape_htmlusage and docstring; nothing to change here.
148-152: Docstring wording tweak is fine, but keep an eye on consistencyThe updated
Returns:text (“A queryset of sponsors, or None if an error occurs.”) remains accurate forget_sponsors_data(limit: int = 10) -> QuerySet | None. No functional or typing concerns here.
236-247: strip_markdown typing and pattern annotation look good
strip_markdown(text: str) -> strandslack_link_pattern: re.Pattern[str]accurately describe the function behavior and improve clarity; no issues noted.
rudransh-shrivastava
left a comment
rudransh-shrivastava
left a comment
There was a problem hiding this comment.
Hi, I have suggested changes for backend/apps/common/utils.py only, please apply similar changes to the other two files too.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
backend/apps/slack/utils.py (4)
93-93: Remove local variable type hints per previous review feedback.Based on the past review discussion, the project convention is to limit type hints to function boundaries (parameters and return types) rather than annotating obvious local variables. Please remove the type annotations for
response,items_total, anditems.Apply this diff:
- response: requests.Response = requests.get(OWASP_NEWS_URL, timeout=timeout) + response = requests.get(OWASP_NEWS_URL, timeout=timeout) tree = html.fromstring(response.content) h2_tags = tree.xpath("//h2") - items_total: int = 0 - items: list[dict[str, str]] = [] + items_total = 0 + items = []Also applies to: 97-98
128-128: Remove local variable type hint per project convention.The type annotation for
file_pathis unnecessary since it's obviously a string literal. Per previous feedback, please limit type hints to function signatures.Apply this diff:
- file_path: str = "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/staff.yml" + file_path = "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/staff.yml"
193-193: Remove local variable type hint per project convention.Please remove the type annotation for
textto align with the project's approach of annotating only function boundaries.Apply this diff:
- text: list[str] = [] + text = []
246-246: Remove local variable type hint per project convention.While the
re.Pattern[str]annotation is more explicit than simpler cases, the project convention is to avoid annotating local variables where the type is clear from context.Apply this diff:
- slack_link_pattern: re.Pattern[str] = re.compile(r"<(https?://[^|]+)\|([^>]+)>") + slack_link_pattern = re.compile(r"<(https?://[^|]+)\|([^>]+)>")backend/apps/common/utils.py (5)
58-58: Verify docstring convention for optional parameters.Based on past review discussions, there's an ongoing question about whether to use
(str | None)or(str, optional)in docstrings. The reviewer previously requested consistency with existing project code, suggesting(str, optional)format. Please verify the established project convention.#!/bin/bash # Check existing docstring conventions for optional parameters across the project rg -n -C2 -g '*.py' '^\s+\w+\s+\([^)]*,\s*optional\)' backend/apps/ | head -40
106-109: Remove local variable type hint per project convention.The type annotation for
x_forwarded_forshould be removed to align with project conventions. However, the logic refactoring itself is fine.Apply this diff:
- x_forwarded_for: str | None = request.META.get("HTTP_X_FORWARDED_FOR") - if x_forwarded_for: - return x_forwarded_for.split(",")[0] - return request.META.get("REMOTE_ADDR", "") + x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") + if x_forwarded_for: + return x_forwarded_for.split(",")[0] + return request.META.get("REMOTE_ADDR", "")
154-154: Remove local variable type hint per project convention.Please remove the type annotation for
dtto align with project standards.Apply this diff:
- dt: datetime = datetime.strptime(value, "%Y-%m-%d").replace(tzinfo=UTC) + dt = datetime.strptime(value, "%Y-%m-%d").replace(tzinfo=UTC)
224-224: Verify docstring convention for optional parameters.Similar to the
clean_urlfunction, please verify whether the project convention is(str | None)or(str, optional)for consistency.
168-168: Fix spacing in docstring parameter.Missing space after comma in the type specification.
Apply this diff:
- unit (str,optional): The unit to append. + unit (str, optional): The unit to append.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/common/utils.py(9 hunks)backend/apps/slack/utils.py(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/common/utils.py
🔇 Additional comments (6)
backend/apps/slack/utils.py (2)
9-9: LGTM! Type hints added correctly.The
Anyimport and function signature forescapeare properly typed.Also applies to: 25-25
183-183: LGTM! Proper type hint for blocks parameter.The tuple type hint correctly captures that blocks is a tuple of dictionaries.
backend/apps/common/utils.py (4)
8-8: LGTM! Proper use of TYPE_CHECKING.Using
TYPE_CHECKINGto importHttpRequestis the correct pattern to avoid runtime circular import issues while maintaining type safety.Also applies to: 17-19
93-93: LGTM! Proper type hint for HttpRequest parameter.The addition of
HttpRequesttype hint improves type safety and IDE support for this function.
143-143: LGTM! Expanded parameter types correctly.Adding
datetimeto the accepted parameter types is a good enhancement, and the docstring correctly reflects this.Also applies to: 147-147
163-163: LGTM! Optional parameter typed correctly.The
unitparameter properly usesstr | Noneto indicate it's optional.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Let's keep it simple and handle function/method level hints only in this PR. Please use optional -- we can later migrate it to something like str | None -- could be your next PR @dhirukumar
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 (3)
backend/apps/slack/utils.py (3)
81-114: Fix docstring to match type hint.The type hint specifies
timeout: float | None = 30, but the docstring at line 87 statestimeout (int, optional). These should be consistent.Apply this diff to fix the docstring:
- timeout (int, optional): The request timeout in seconds. + timeout (float, optional): The request timeout in seconds.The local variable type annotations are correct.
118-141: Fix docstring to match type hint.Similar to
get_news_data, the type hint specifiestimeout: float | None = 30, but the docstring at line 122 statestimeout (int, optional).Apply this diff to fix the docstring:
- timeout (int, optional): The request timeout in seconds. + timeout (float, optional): The request timeout in seconds.The
file_pathtype annotation and function return type are correct.
236-247: Fix docstring return format for consistency.The return section of the docstring is missing the type prefix. For consistency with other functions in the file, it should include the return type.
Apply this diff to fix the docstring:
Returns: - The text with markdown formatting removed. + str: The text with markdown formatting removed.The type annotation for
slack_link_pattern: re.Pattern[str]is correct.
🧹 Nitpick comments (1)
backend/apps/github/utils.py (1)
60-82: Good improvements to error handling structure.The addition of the
else:block is good practice, ensuring the return only happens when no exception occurs. Type annotations and docstring updates accurately reflect the function's behavior.Minor note: The default value
30is anint, while the type hint isfloat | None. This works due to Python's int/float compatibility but could be more explicit as30.0for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/utils.py(8 hunks)backend/apps/github/utils.py(4 hunks)backend/apps/slack/utils.py(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/common/utils.py
🔇 Additional comments (6)
backend/apps/github/utils.py (2)
36-36: LGTM! Type hint and docstring properly aligned.The optional parameter type hint
str | Nonecorrectly reflects the nullable target parameter, and the docstring format matches the project conventions.Also applies to: 41-41
84-96: LGTM! Docstring formatting updates are appropriate.The docstring return type formatting has been updated consistently across both functions.
Also applies to: 99-123
backend/apps/slack/utils.py (4)
9-9: LGTM! Correct type hints added.The addition of
Anyto typing imports supports the type annotations used throughout the file. Theescapefunction's type hints are accurate.Also applies to: 25-35
56-56: LGTM! Appropriate use ofAnyfor external API response.The return type
list[dict[str, Any]]correctly reflects the structure of GSoC project data from the search API.
144-161: LGTM! Function signature and docstring are clear.The simplified docstring return description is acceptable, as the type information is captured in the function signature.
183-233: LGTM! Type hints accurately represent the data structures.The parameter type
tuple[dict[str, Any], ...]correctly represents Slack block elements, and the local variable annotationtext: list[str]is accurate.
|
Hi @rudransh-shrivastava, |
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/common/utils.py (1)
93-108: <!-- [scratchpad_start]
[task verification_1/1]
[observations]
- Function: get_user_ip_address(request: HttpRequest) -> str (lines 93-107)
- Return annotation: str (but actual return can be None)
- Return statement:
x_forwarded_for.split(",")[0] if x_forwarded_for else request.META.get("REMOTE_ADDR")- Tests found:
- test_get_user_ip_address: provides mock_request with HTTP_X_FORWARDED_FOR or REMOTE_ADDR keys present
- test_get_user_ip_address_local: provides IS_LOCAL_ENVIRONMENT=True case
- CRITICAL: Tests do NOT cover the case where BOTH keys are missing (where None would be returned)
- Usage in algolia.py:
- Line 50: assigns result to ip_address variable
- Line 57: uses ip_address in f-string cache key:
f"{cache_key}:{ip_address}"- Line 114: passes to IndexBase.get_client(ip_address=ip_address)
- search function parameter (line 87):
ip_address=Nonedefaults to None- search function docstring (line 97): says "ip_address (str, optional)" - acknowledges optional
[analysis]
- Review comment is CORRECT and verified
- Type annotation -> str is violated: request.META.get() returns None if key not found
- No test covers the edge case where both keys are absent
- Usage in algolia.py line 57 would produce "{cache_key}:None" string if None is returned
- search function already accepts None (line 87), so inconsistency is real
[pending]
None - verification complete[actions]
Generate rewritten review comment confirming the issue with categorization tag[done]
Issue confirmed: function returns None in uncovered edge case, violating str type annotation
</scratchpad_end] -->Fix return type inconsistency in
get_user_ip_addressThe function annotation promises
-> str, butrequest.META.get("REMOTE_ADDR")can returnNonewhen neitherHTTP_X_FORWARDED_FORnorREMOTE_ADDRkeys exist in the request metadata. Current tests don't cover this case.To resolve, either:
- Change return type to
str | Noneand update the docstring, or- Provide a safe default (e.g.,
""or"0.0.0.0") to guarantee a string is always returned.Note: Line 57 in
backend/apps/core/api/internal/algolia.pyincludesip_addressin an f-string cache key, which would produce"...cache_key:None"if the value isNone—likely unintended behavior.
🧹 Nitpick comments (2)
backend/apps/github/utils.py (1)
60-79: Clarify semantics oftimeoutwhenNoneis passedThe
timeoutparameter is now typed asfloat | Noneand documented as optional, but the docstring doesn’t explain whatNonemeans (effectively “no timeout” forrequests.get). Consider documenting this behavior so callers know when to passNonevs relying on the default.backend/apps/common/utils.py (1)
141-159: natural/round helpers’ type hints and docstrings match behavior
natural_date(value: int | str | datetime) -> strcorrectly describes the supported types and the existing branching logic.natural_number(value: int, unit: str | None = None) -> strplus the(str, optional)docstring forunitare consistent with how the unit is appended and pluralized.- Updated param type details in the
round_downdocstring match the existing function signature and behavior.These helpers’ public contracts now read clearly without altering runtime logic.
Also applies to: 161-173, 176-187
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/utils.py(8 hunks)backend/apps/github/utils.py(4 hunks)backend/apps/slack/utils.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/common/utils.py
🔇 Additional comments (4)
backend/apps/github/utils.py (2)
36-56: check_funding_policy_compliance: type hint and docstring are consistent
targetbeing annotated asstr | Noneand documented as(str, optional)matches the early return on falsytargetand doesn’t change logic. This looks correct and aligned with prior review feedback on docstring style.
82-95: Return type unionsstr | Noneaccurately reflect failure casesExposing
str | Nonefor bothget_repository_pathandnormalize_urlmatches the implementation (they can fail and return no usable string), and the docstrings clearly document theNonecases. From this file’s perspective, these annotations look sound; just ensure call sites are prepared to handleNoneif they weren’t already.Also applies to: 97-121
backend/apps/common/utils.py (2)
8-9: Good use ofTYPE_CHECKINGand forward-referencedHttpRequestImporting
HttpRequestonly underTYPE_CHECKINGtogether withfrom __future__ import annotationskeeps runtime imports minimal while giving static checkers full type info. This is a clean pattern and fits the project’s typing goals.Also applies to: 17-19
54-68: Optional URL handling inclean_urlandvalidate_urlis consistentAllowing
url: str | Nonein bothclean_urlandvalidate_urland documenting as(str, optional)aligns their signatures and behavior.clean_urlreturningNonefor falsy/empty input, andvalidate_urlreturningFalsewhenurlis falsy or parsing fails, form a coherent pair of helpers with clear, predictable semantics.Also applies to: 218-236
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/apps/common/utils.py (1)
93-107: get_user_ip_address(): annotation is clear; consider ensuring a non-None fallbackTyping the parameter as
HttpRequestand the return asstrimproves clarity. One minor point:request.META.get("REMOTE_ADDR")can technically beNone, which would violate the-> strguarantee in edge cases. You might consider a default, e.g.:- x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") - return x_forwarded_for.split(",")[0] if x_forwarded_for else request.META.get("REMOTE_ADDR") + x_forwarded_for = request.META.get("HTTP_X_FORWARDED_FOR") + return ( + x_forwarded_for.split(",")[0] + if x_forwarded_for + else request.META.get("REMOTE_ADDR", "") + )to always return a string.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/utils.py(8 hunks)backend/apps/github/utils.py(4 hunks)backend/apps/slack/utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/github/utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
Repo: OWASP/Nest PR: 1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/common/utils.py
🧬 Code graph analysis (1)
backend/apps/common/utils.py (2)
backend/apps/github/models/release.py (1)
url(64-66)backend/apps/github/models/repository.py (1)
url(192-194)
🔇 Additional comments (10)
backend/apps/slack/utils.py (3)
25-35: escape(): type signature is precise and consistentAnnotating
escape(content: str) -> strmatches the existing behavior and clarifies the expected input and output without changing semantics.
80-92: get_news_data(): timeout docstring matches annotated intentThe
timeout (float, optional)docstring is now consistent with thetimeout: float | None = 30annotation and the “optional” style used elsewhere in the project.
117-137: get_staff_data(): timeout documentation aligns with typingThe updated
timeout (float, optional)docstring aligns with the annotatedtimeout: float | None = 30and mirrors the style used in other utilities.backend/apps/common/utils.py (7)
8-19: TYPE_CHECKING import and HttpRequest guard are idiomaticUsing
TYPE_CHECKINGwith a guardedHttpRequestimport works well withfrom __future__ import annotations, keeping runtime imports minimal while giving type checkers full context.
54-68: clean_url(): annotations now match actual None semanticsThe signature
clean_url(url: str | None) -> str | Noneand the updated docstring correctly express that falsy/empty inputs and fully stripped values yieldNone, aligning types with existing behavior.
83-90: get_nest_user_agent(): return typing and docstring are alignedExplicitly annotating
-> strand documenting the return type clarifies the contract without changing behavior.
141-158: natural_date(): union typing and docstring accurately describe supported inputsThe
value: int | str | datetimeannotation plus the updated docstring match the branching logic (string parse, timestamp, or datetime passthrough) and make the API surface clearer.
161-173: natural_number(): optional unit typing matches behaviorThe signature
natural_number(value: int, unit: str | None = None) -> strand theunit (str, optional)docstring correctly reflect that the unit is purely additive and omitted whenNone.
176-187: round_down(): docstring now fully specifies parameter typesAdding
(int)types forvalueandbasein the docstring matches the function signature and clarifies the expected arguments.
218-236: validate_url(): optional URL typing lines up with existing checksThe updated signature
validate_url(url: str | None) -> boolandurl (str, optional)docstring align with the earlyif not url: return Falselogic and downstream parsing, accurately documenting the function’s contract.
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 (2)
backend/apps/github/utils.py(5 hunks)backend/apps/slack/utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/slack/utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend checks
🔇 Additional comments (4)
backend/apps/github/utils.py (4)
36-36: LGTM: Optional target parameter correctly typed.The type hint
target: str | Noneand docstring format(str, optional)correctly reflect that the target parameter can be None, and the function logic properly handles this case.Also applies to: 41-41
72-72: LGTM: Return type correctly reflects non-optional string.The docstring correctly states that an empty string is returned on failure, which aligns with the
-> strreturn type annotation and the actual implementation.
82-82: LGTM: Optional return type correctly annotated.The return type
str | Noneand updated docstring accurately reflect that the function can return None when URL parsing fails, matching the implementation at line 94.Also applies to: 89-90
97-97: LGTM: Optional return type correctly annotated.The return type
str | Noneand docstring properly indicate that None is returned for invalid URLs, which aligns with the implementation.Also applies to: 105-105
Head branch was pushed to by a user without write access
|
3 participants
✨ Proposed Change
Resolves: #2650
This Pull Request introduces complete type hints across all backend utility functions, improving code clarity, static analysis, and maintainability.
🔧 Changes Implemented
None📁 Updated Files
backend/apps/common/utils.pybackend/apps/github/utils.pybackend/apps/slack/utils.py📌 Example Followed
✅ Checklist