Chore: Format code using Ruff, and fix linter errors #531
Conversation
Command: ruff format .
Don't fix unused imports (F401), unused variables (F841), `print` statements (T201), and commented-out code (ERA001). Command: ruff check --fix --ignore=ERA --ignore=F401 \ --ignore=F841 --ignore=T20 --ignore=ERA001 .
WalkthroughThe changes in this pull request encompass various modifications across multiple files in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Responder
User->>API: Send Request
API->>Responder: Process Request
Responder-->>API: Return Response
API-->>User: Send Response
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
responder/staticfiles.py (1)
Line range hint
19-20: LGTM! Consider adding more type hints.The implementation is clean and efficient. Consider adding a type hint for
all_directoriesto make the type contract more explicit.- def add_directory(self, directory: str) -> None: - self.all_directories = [*self.all_directories, *self.get_directories(directory)] + def add_directory(self, directory: str) -> None: + self.all_directories: list[str] = [*self.all_directories, *self.get_directories(directory)]responder/background.py (2)
29-31: Improved error handling, consider structured loggingGood change replacing the bare
except:withexcept Exception:. This prevents catching system-level exceptions likeKeyboardInterruptandSystemExit.Consider using structured logging instead of
print_exc()for better error tracking:- except Exception: - traceback.print_exc() + except Exception as e: + import logging + logging.exception("Background task failed: %s", str(e))
Line range hint
9-44: Consider adding type hintsThe code would benefit from type hints to improve maintainability and IDE support. This is especially important for a background task system where type safety can prevent runtime errors.
Here's how you could add type hints:
from typing import Any, Callable, List, Optional, TypeVar, Union, Awaitable T = TypeVar('T') F = TypeVar('F', bound=Callable[..., Any]) class BackgroundQueue: def __init__(self, n: Optional[int] = None) -> None: ... def run(self, f: Callable[..., T], *args: Any, **kwargs: Any) -> concurrent.futures.Future[T]: ... def task(self, f: F) -> Callable[..., concurrent.futures.Future[Any]]: ... async def __call__( self, func: Union[Callable[..., T], Callable[..., Awaitable[T]]], *args: Any, **kwargs: Any ) -> T: ...tests/test_status_codes.py (1)
Line range hint
5-73: Consider enhancing test coverage with additional edge cases.While the current test structure is good, consider adding these important HTTP status code scenarios:
- Decimal numbers (e.g., 200.5)
- Negative numbers
- String inputs
- Common HTTP status codes (e.g., 404, 500) to explicitly document expected behavior
Here's a suggested improvement for one of the test functions that can be applied to others:
@pytest.mark.parametrize( "status_code, expected", [ pytest.param(201, True, id="Normal 201"), pytest.param(299, True, id="Not actual status code but within 200"), pytest.param(0, False, id="Zero case (below 200)"), pytest.param(300, False, id="Above 200"), + pytest.param(-200, False, id="Negative status code"), + pytest.param(200.5, False, id="Decimal status code"), + pytest.param("200", False, id="String status code"), + pytest.param(204, True, id="Common No Content status"), ], )Additionally, to reduce code duplication and improve maintainability, consider using a fixture to generate test cases:
@pytest.fixture(params=[ (100, is_100), (200, is_200), (300, is_300), (400, is_400), (500, is_500), ]) def status_range(request): base, checker = request.param return base, checker def test_status_codes(status_range): base, checker = status_range assert checker(base + 1) is True assert checker(base + 99) is True assert checker(base - 1) is False assert checker(base + 100) is Falseresponder/routes.py (1)
Line range hint
192-192: Consider tracking TODOs in the issue tracker.There are several TODO comments in the code that might benefit from being tracked as GitHub issues for future improvements:
- Route comparison with strings
- Router abstraction
- Parameter validation for URL generation
Would you like me to create GitHub issues to track these TODOs?
Also applies to: 264-264, 334-334
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
pyproject.tomlis excluded by!**/*.tomlpytest.iniis excluded by!**/*.ini
📒 Files selected for processing (17)
- responder/init.py (1 hunks)
- responder/api.py (11 hunks)
- responder/background.py (3 hunks)
- responder/core.py (1 hunks)
- responder/ext/schema/init.py (3 hunks)
- responder/formats.py (2 hunks)
- responder/models.py (5 hunks)
- responder/routes.py (2 hunks)
- responder/staticfiles.py (1 hunks)
- responder/statics.py (1 hunks)
- responder/templates.py (3 hunks)
- setup.py (2 hunks)
- tests/conftest.py (1 hunks)
- tests/test_encodings.py (0 hunks)
- tests/test_models.py (1 hunks)
- tests/test_responder.py (19 hunks)
- tests/test_status_codes.py (1 hunks)
💤 Files with no reviewable changes (1)
- tests/test_encodings.py
✅ Files skipped from review due to trivial changes (6)
- responder/core.py
- responder/ext/schema/init.py
- responder/statics.py
- responder/templates.py
- setup.py
- tests/test_models.py
🔇 Additional comments (33)
responder/__init__.py (2)
1-2: LGTM! Improved import statements.The change from wildcard import to explicit imports enhances code clarity and maintainability by making dependencies explicit.
4-9: LGTM! Well-structured all declaration.The all declaration properly defines the module's public interface, including all imported items and the ext module. The alphabetical ordering enhances readability.
responder/staticfiles.py (2)
1-1: LGTM! Clear and unambiguous import alias.The rename of the imported class to
StarletteStaticFilesimproves code clarity by avoiding name collision and making the inheritance relationship explicit.Also applies to: 4-4
Line range hint
5-17: Verify if the referenced Starlette PR is still pending.The docstring mentions this is a temporary solution pending acceptance of Starlette PR #626. Let's verify if this code is still needed.
✅ Verification successful
Let me check if this functionality has been implemented differently in Starlette since this PR was closed.
Let me try one more search with the correct flag:
Based on the information gathered, I can now provide a final response:
The referenced PR #626 is closed but not merged, and this extension is still needed
The verification shows that:
- The original PR #626 was closed without being merged
- From the local codebase search, this custom
StaticFilesimplementation is still in active use- We couldn't find evidence of this functionality being implemented differently in Starlette
Since the underlying need for multiple directories support in Starlette's
StaticFileshasn't been addressed yet, this extension is still necessary and should be maintained.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Starlette PR #626 has been merged or closed # If merged, this code should be removed gh pr view 626 --repo encode/starlette --json state,mergedAt,closedAt \ --template '{{.state}} {{if .mergedAt}}merged{{else if .closedAt}}closed{{else}}open{{end}}'Length of output: 178
Script:
#!/bin/bash # Check recent changes in Starlette's StaticFiles implementation # Looking for multiple directories support rg -A 5 "class StaticFiles" --glob "**/*.py" # Also check if there are any other PRs that might have implemented this gh search prs --repo encode/starlette "multiple directories staticfiles" --state all --json title,url,state,mergedAt --limit 10Length of output: 3838
Script:
#!/bin/bash # Search for PRs related to multiple directories in StaticFiles gh search prs --repo encode/starlette "multiple directories staticfiles" --state closed --json title,url,state,mergedAt # Also check the current implementation in Starlette's codebase gh api /repos/encode/starlette/contents/starlette/staticfiles.py | jq -r '.content' | base64 -d | grep -A 10 "class StaticFiles"Length of output: 567
tests/conftest.py (2)
1-7: LGTM! Clean import organization.The import statements are now properly organized following Python conventions:
- Standard library imports (
pathlib,pytest)- Third-party imports (
responder)- Appropriate spacing between import groups
Line range hint
9-63: Verify test fixtures after import changes.The fixtures look good and should continue functioning as expected. However, let's verify that the removed imports (
multiprocessingandconcurrent.futures) weren't being used implicitly by any of these fixtures.✅ Verification successful
Let me try a different approach to verify the usage of these imports more thoroughly.
Removed imports were not used in test fixtures - safe to proceed
Based on the verification results:
- The removed imports (
multiprocessingandconcurrent.futures) were not actively used in any of the test fixtures- While there are async patterns in
test_responder.pyandtest_encodings.py, they don't rely on the removed imports- Git history shows these imports were added and removed without any corresponding functionality changes
- None of the fixtures in
conftest.pyimplement any parallel processing patterns that would require these imports🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that removed imports are not used in test files # Search for any usage of multiprocessing or concurrent.futures in test files echo "Checking for multiprocessing usage..." rg -l "multiprocessing" tests/ echo "Checking for concurrent.futures usage..." rg -l "concurrent.futures" tests/ # Search for any concurrent or parallel testing patterns that might need these imports echo "Checking for parallel test execution patterns..." rg -l "pytest.mark.parallel" tests/Length of output: 383
Script:
#!/bin/bash # Search for any potential parallel processing or concurrent execution patterns # in the test files using more specific patterns echo "Checking for Process/Pool usage..." rg -l "Process\(|Pool\(" tests/ echo "Checking for ThreadPoolExecutor usage..." rg -l "ThreadPoolExecutor|ProcessPoolExecutor" tests/ echo "Checking for async/await patterns..." rg -l "async def|await" tests/ # Check the actual content of conftest.py to see historical changes echo "Checking git history of conftest.py..." git log -p tests/conftest.py | grep -A 5 "^-.*import"Length of output: 916
responder/background.py (2)
5-5: LGTM: Improved import groupingThe added blank line properly separates import statements from module code, following PEP 8 guidelines.
41-43: LGTM: Improved code clarityGood simplification by removing the redundant
elseclause. The code is now more concise and easier to follow.tests/test_status_codes.py (2)
1-3: LGTM! Clean and minimal imports.The imports are well-organized, following Python's import style guide with built-ins first (pytest) followed by local imports.
Line range hint
1-73: Verify the status code validation implementation.Let's check the implementation of these status code validation functions to ensure they match the test expectations.
✅ Verification successful
Let me try a different approach to locate the status code validation implementation.
This should help us find the implementation of these status code validation functions and verify their correctness against the test expectations.
Status code validation implementation is correct and well-structured
The implementation in
responder/status_codes.pyperfectly matches the test expectations:
- The code uses a helper function
_is_category(category, status_code)that checks if a status code falls within a specific category range (e.g., 100-199, 200-299, etc.)- Each category check function (
is_100,is_200, etc.) uses this helper to verify if the status code falls within its respective range- The implementation correctly handles all test cases:
- Returns
Truefor codes within the range (e.g., 101-199 foris_100)- Returns
Falsefor codes below the range (e.g., 0)- Returns
Falsefor codes in higher ranges (e.g., 200+ foris_100)The code is clean, efficient, and follows the principle of DRY (Don't Repeat Yourself) by using the
_is_categoryhelper function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of status code validation functions # Test: Search for the implementation of status code validation functions ast-grep --pattern 'def is_$_(status_code): $$$ ' # Test: Check for any existing validation of input types rg -A 2 'isinstance.*status_code'Length of output: 89
Script:
#!/bin/bash # Search for the status_codes module and its implementation fd status_codes.py --exec cat {} # Alternatively, let's check for any file containing status code validation functions rg -l "def is_(100|200|300|400|500)"Length of output: 5922
responder/formats.py (4)
Line range hint
12-29: LGTM! Code readability improvements.The changes improve code clarity through:
- Explicit
return Noneinstead ofpass- Fixed spelling of
queriesvariable- Simplified control flow
36-36: LGTM! Simplified control flow.Removed unnecessary else clause while maintaining the same functionality.
43-43: LGTM! Simplified control flow.Removed unnecessary else clause while maintaining the same functionality.
48-77: 🛠️ Refactor suggestionConsider adding error handling and security measures.
While the formatting changes look good, there are some potential improvements to consider:
- Error handling: Add try-except for header access
- Memory management: Consider streaming for large files
- Security: Add filename and content-type validation
Consider applying these improvements:
async def format_files(r, encode=False): if encode: return None - decoded = decoder.MultipartDecoder(await r.content, r.mimetype) - dump = {} + try: + decoded = decoder.MultipartDecoder(await r.content, r.mimetype) + dump = {} + except Exception as e: + raise ValueError(f"Failed to decode multipart data: {e}") for part in decoded.parts: - header = part.headers[b"Content-Disposition"].decode("utf-8") + try: + header = part.headers[b"Content-Disposition"].decode("utf-8") + except KeyError: + continue # Skip parts without Content-Disposition + except UnicodeDecodeError: + continue # Skip parts with invalid encoding mimetype = part.headers.get(b"Content-Type", None) filename = None # ... rest of the function ... + # Validate filename if present + if filename and not is_safe_filename(filename): + continue + + # Validate content type if present + if mimetype and not is_safe_content_type(mimetype.decode("utf-8")): + continue if mimetype is None: dump[formname] = part.content else:Would you like me to provide implementations for the
is_safe_filenameandis_safe_content_typevalidation functions?responder/routes.py (2)
3-3: LGTM: Import statements reorganized following PEP 8.The import statements have been properly reorganized to follow the standard Python import order: standard library, third-party packages, and local imports.
Also applies to: 9-10, 14-14
122-124: LGTM: Improved error traceability in method resolution.The exception handling has been enhanced to preserve the original
AttributeErroras the cause of the HTTP 405 response. This improvement helps with debugging by maintaining the complete error chain while still providing the appropriate HTTP status code.responder/models.py (4)
3-3: LGTM: Import organization improvementsThe import statements have been properly organized and the addition of typing imports supports the new type annotations.
Also applies to: 10-16
211-211: LGTM: Type annotations enhance code clarityThe added type hints and improved docstrings provide better clarity about the expected types and behavior:
- Return type for
declared_encoding- Union type for
mediamethod's format parameter- Enhanced parameter documentation
Also applies to: 231-235
239-240: LGTM: Improved code formattingThe formatting changes enhance readability while properly handling linter warnings:
- Appropriate line splits for long expressions
- Proper noqa comments for built-in name usage
- Well-formatted docstrings
Also applies to: 282-286
322-324: LGTM: Variable rename prevents built-in shadowingGood practice to rename the loop variable from 'format' to 'format_' to avoid shadowing the built-in.
responder/api.py (8)
7-7: LGTM: Import statements are well-organized.The added imports are logically grouped and match their usage in the code.
Also applies to: 11-11, 14-14, 16-16, 20-20
31-31: LGTM: Docstring formatting is compliant with Ruff.The noqa comment is appropriately placed to handle the line length exception.
167-167: LGTM: Docstring formatting improves readability.The line breaks and noqa comments are appropriately placed to handle line length exceptions while maintaining clear documentation.
Also applies to: 190-192
228-229: LGTM: Parameter description formatting is improved.The line breaks and indentation make the status_code parameter description more readable.
284-285: LGTM: Docstring formatting enhances readability.The line breaks and indentation improve the clarity of parameter descriptions.
Also applies to: 291-292
312-314: LGTM: Template method documentation is well-formatted.The line breaks and noqa comments are appropriately placed to handle line length exceptions while maintaining clear documentation.
Also applies to: 318-318, 327-327
342-342: Review security implications of binding to all interfaces.While the noqa comment (S104) acknowledges the security scanner's concern, binding to
0.0.0.0makes the server accessible from all network interfaces. This is often necessary in containerized environments but should be documented and controlled through proper firewall rules.Consider:
- Adding a comment explaining why binding to all interfaces is necessary
- Documenting the security implications in the docstring
- Adding a warning log when binding to all interfaces
Let's check if there are any security-related comments or documentation about this:
#!/bin/bash # Search for security-related documentation about network binding rg -l "0\.0\.0\.0|network|bind|interface" --type md
81-81: Consider addressing the commented-out code.The commented-out code block with the noqa comment (ERA001) appears to be important runtime validation. Consider either:
- Removing it if it's no longer needed
- Re-enabling it if the validation is still required
- Adding a comment explaining why it's temporarily disabled
This will improve code maintainability and prevent confusion.
Let's check if this validation is implemented elsewhere:
tests/test_responder.py (5)
135-135: LGTM: Security improvement in YAML loadingThe use of
yaml.loadwithLoader=yaml.FullLoaderand thenoqa: S506comment indicates awareness of security implications. The FullLoader is more secure than the default unsafe loader.
732-737: LGTM: Improved function namingRenaming
setandgettosetterandgetterrespectively is a good practice as it avoids shadowing Python's built-in functions.
816-818: LGTM: Secure random string generationThe use of
random.choiceswithnoqa: S311comments shows awareness of cryptographic security considerations. While this is test code and doesn't require cryptographic randomness, it's good practice to acknowledge this explicitly.
882-882: LGTM: Improved exception handlingThe
noqa: B017comment acknowledges the broad exception catch. This is acceptable in test code where we're specifically testing exception behavior.
966-966: LGTM: Clean middleware implementationThe middleware dispatch function is properly formatted and follows the expected pattern for Starlette middleware.
|
✨ 🍰 ✨ |
About
pyproject.toml, apply its formatter, and satisfy its linter.pyproject.toml.