Blacken most apps #8902
Blacken most apps #8902
Conversation
These apps have either no or very few PRs that update them, so this seems like a good time to reformat them. This commit applies black with the `--skip-string-normalization` option to make it easier to review; I'll normalize the strings in the next commit.
WalkthroughThis pull request introduces a comprehensive set of formatting and stylistic changes across multiple Django applications within the CVAT project. The modifications primarily focus on standardizing string quotation from single quotes to double quotes, improving code readability through consistent formatting, and adding minor enhancements to application initialization processes. No significant functional changes were introduced, maintaining the existing logic and behavior of the codebase. Changes
Poem
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 comments (1)
cvat/apps/events/event.py (1)
Line range hint
45-77: Add input validation and error handling.The function should validate that the
scopeparameter matches one of the allowed event scopes defined inEventScopes.RESOURCES. Additionally, consider adding error handling for JSON serialization failures.Here's a suggested implementation:
def record_server_event( *, scope: str, request_id: Optional[str], payload: Optional[dict] = None, on_commit: bool = False, **kwargs, ) -> None: + # Validate scope + resource, action = scope.split(":", 1) + if resource not in EventScopes.RESOURCES or action not in EventScopes.RESOURCES[resource]: + raise ValueError(f"Invalid event scope: {scope}") + payload = payload or {} payload_with_request_id = { **payload, "request": { **payload.get("request", {}), "id": request_id, }, } data = { "scope": scope, "timestamp": str(datetime.now(timezone.utc).timestamp()), "source": "server", - "payload": JSONRenderer().render(payload_with_request_id).decode("UTF-8"), + "payload": JSONRenderer().render(payload_with_request_id).decode("UTF-8"), **kwargs, } - rendered_data = JSONRenderer().render(data).decode("UTF-8") + try: + rendered_data = JSONRenderer().render(data).decode("UTF-8") + except Exception as e: + vlogger.error(f"Failed to render event data: {e}") + return
🧹 Nitpick comments (7)
cvat/apps/events/event.py (2)
45-47: Add docstring to explain function purpose and parameters.The function lacks documentation explaining its purpose, parameters, and usage examples.
Add a docstring like this:
def record_server_event( *, scope: str, request_id: Optional[str], payload: Optional[dict] = None, on_commit: bool = False, **kwargs, ) -> None: + """ + Record a server-side event with the specified scope and payload. + + Args: + scope: The event scope in format "action:resource" + request_id: Optional request identifier + payload: Optional event payload dictionary + on_commit: If True, defer logging until transaction commit + **kwargs: Additional fields to include in the event data + + Example: + >>> record_server_event( + ... scope="create:task", + ... request_id="req-123", + ... payload={"task_id": 42}, + ... on_commit=True + ... ) + """
Line range hint
45-85: Consider reorganizing code structure.The
record_server_eventfunction is placed between class definitions, which breaks the logical grouping. Consider moving it after all class definitions to maintain better code organization.The suggested order would be:
- Imports
- Constants and helper functions (
event_scope)- Classes (
EventScopes,EventScopeChoice,AllEvents)- Main functions (
record_server_event)cvat/apps/iam/rules/tests/generate_tests.py (1)
39-41: Consider using Path objects for consistencyWhile the formatting change looks good, consider using Path objects consistently instead of converting to relative path strings.
- [sys.executable, generator_path.relative_to(rules_dir), "tests/configs"], cwd=rules_dir + [sys.executable, generator_path.relative_to(rules_dir), Path("tests/configs")], cwd=rules_dircvat/apps/lambda_manager/serializers.py (1)
46-50: Consider adding validation for mapping structureWhile the serializer implementation is good, consider adding custom validation to ensure the mapping structure matches expected format.
Example validation:
def validate_mapping(self, value): """Validate the mapping structure matches expected format.""" for label_name, mapping in value.items(): if not isinstance(label_name, str): raise serializers.ValidationError("Label names must be strings") return valuecvat/apps/webhooks/signals.py (1)
Line range hint
41-67: Consider adding retry mechanism for failed webhook deliveries.The webhook delivery implementation looks solid with proper security (HMAC signatures) and error handling. However, consider implementing a retry mechanism with exponential backoff for failed deliveries (status codes >= 400) to improve reliability.
pyproject.toml (1)
20-20: Consider documenting the roadmap for removing exclusions.The TODO comment indicates a plan to remove these exclusions. Consider documenting the criteria or timeline for removing each exclusion to help track progress.
cvat/apps/log_viewer/views.py (1)
22-25: Consider making the redirect URL configurable.The hardcoded localhost URL might not be suitable for all development environments. Consider making this URL configurable through Django settings.
- return HttpResponsePermanentRedirect("http://localhost:3001/dashboards") + return HttpResponsePermanentRedirect(settings.DEBUG_DASHBOARD_URL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (77)
cvat/apps/dataset_repo/migrations/0001_initial.py(1 hunks)cvat/apps/dataset_repo/migrations/0002_auto_20190123_1305.py(1 hunks)cvat/apps/dataset_repo/migrations/0003_gitdata_lfs.py(1 hunks)cvat/apps/dataset_repo/migrations/0004_rename.py(1 hunks)cvat/apps/dataset_repo/migrations/0005_auto_20201019_1100.py(1 hunks)cvat/apps/dataset_repo/migrations/0006_gitdata_format.py(1 hunks)cvat/apps/events/apps.py(1 hunks)cvat/apps/events/cache.py(1 hunks)cvat/apps/events/const.py(1 hunks)cvat/apps/events/event.py(3 hunks)cvat/apps/events/export.py(5 hunks)cvat/apps/events/handlers.py(15 hunks)cvat/apps/events/permissions.py(2 hunks)cvat/apps/events/rules/tests/generators/events_test.gen.rego.py(1 hunks)cvat/apps/events/serializers.py(2 hunks)cvat/apps/events/tests/test_events.py(3 hunks)cvat/apps/events/urls.py(1 hunks)cvat/apps/events/utils.py(5 hunks)cvat/apps/events/views.py(1 hunks)cvat/apps/health/apps.py(1 hunks)cvat/apps/health/backends.py(1 hunks)cvat/apps/health/management/commands/workerprobe.py(1 hunks)cvat/apps/iam/admin.py(2 hunks)cvat/apps/iam/apps.py(1 hunks)cvat/apps/iam/authentication.py(3 hunks)cvat/apps/iam/filters.py(3 hunks)cvat/apps/iam/forms.py(2 hunks)cvat/apps/iam/middleware.py(2 hunks)cvat/apps/iam/migrations/0001_remove_business_group.py(1 hunks)cvat/apps/iam/models.py(0 hunks)cvat/apps/iam/permissions.py(7 hunks)cvat/apps/iam/rules/tests/generate_tests.py(3 hunks)cvat/apps/iam/schema.py(2 hunks)cvat/apps/iam/serializers.py(6 hunks)cvat/apps/iam/signals.py(3 hunks)cvat/apps/iam/tests/test_rest_api.py(1 hunks)cvat/apps/iam/urls.py(1 hunks)cvat/apps/iam/utils.py(2 hunks)cvat/apps/iam/views.py(8 hunks)cvat/apps/lambda_manager/apps.py(1 hunks)cvat/apps/lambda_manager/permissions.py(1 hunks)cvat/apps/lambda_manager/rules/tests/generators/lambda_test.gen.rego.py(1 hunks)cvat/apps/lambda_manager/serializers.py(2 hunks)cvat/apps/lambda_manager/tests/test_lambda.py(31 hunks)cvat/apps/lambda_manager/urls.py(2 hunks)cvat/apps/lambda_manager/views.py(30 hunks)cvat/apps/log_viewer/apps.py(1 hunks)cvat/apps/log_viewer/permissions.py(2 hunks)cvat/apps/log_viewer/rules/tests/generators/analytics_test.gen.rego.py(3 hunks)cvat/apps/log_viewer/urls.py(1 hunks)cvat/apps/log_viewer/views.py(1 hunks)cvat/apps/organizations/__init__.py(0 hunks)cvat/apps/organizations/admin.py(1 hunks)cvat/apps/organizations/apps.py(1 hunks)cvat/apps/organizations/migrations/0001_initial.py(1 hunks)cvat/apps/organizations/models.py(2 hunks)cvat/apps/organizations/permissions.py(4 hunks)cvat/apps/organizations/rules/tests/generators/invitations_test.gen.rego.py(1 hunks)cvat/apps/organizations/rules/tests/generators/memberships_test.gen.rego.py(2 hunks)cvat/apps/organizations/rules/tests/generators/organizations_test.gen.rego.py(1 hunks)cvat/apps/organizations/serializers.py(3 hunks)cvat/apps/organizations/throttle.py(1 hunks)cvat/apps/organizations/urls.py(1 hunks)cvat/apps/organizations/views.py(4 hunks)cvat/apps/webhooks/apps.py(1 hunks)cvat/apps/webhooks/event_type.py(1 hunks)cvat/apps/webhooks/migrations/0001_initial.py(1 hunks)cvat/apps/webhooks/migrations/0002_alter_webhookdelivery_status_code.py(1 hunks)cvat/apps/webhooks/migrations/0003_alter_webhookdelivery_status_code.py(1 hunks)cvat/apps/webhooks/migrations/0004_alter_webhook_target_url.py(1 hunks)cvat/apps/webhooks/models.py(3 hunks)cvat/apps/webhooks/permissions.py(3 hunks)cvat/apps/webhooks/rules/tests/generators/webhooks_test.gen.rego.py(1 hunks)cvat/apps/webhooks/serializers.py(4 hunks)cvat/apps/webhooks/signals.py(6 hunks)cvat/apps/webhooks/views.py(7 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (2)
- cvat/apps/iam/models.py
- cvat/apps/organizations/init.py
✅ Files skipped from review due to trivial changes (61)
- cvat/apps/iam/migrations/0001_remove_business_group.py
- cvat/apps/dataset_repo/migrations/0001_initial.py
- cvat/apps/log_viewer/urls.py
- cvat/apps/dataset_repo/migrations/0002_auto_20190123_1305.py
- cvat/apps/webhooks/migrations/0002_alter_webhookdelivery_status_code.py
- cvat/apps/health/backends.py
- cvat/apps/dataset_repo/migrations/0004_rename.py
- cvat/apps/dataset_repo/migrations/0005_auto_20201019_1100.py
- cvat/apps/dataset_repo/migrations/0003_gitdata_lfs.py
- cvat/apps/webhooks/migrations/0001_initial.py
- cvat/apps/iam/utils.py
- cvat/apps/events/serializers.py
- cvat/apps/webhooks/rules/tests/generators/webhooks_test.gen.rego.py
- cvat/apps/events/permissions.py
- cvat/apps/organizations/urls.py
- cvat/apps/iam/authentication.py
- cvat/apps/iam/admin.py
- cvat/apps/events/tests/test_events.py
- cvat/apps/iam/forms.py
- cvat/apps/events/urls.py
- cvat/apps/iam/views.py
- cvat/apps/iam/signals.py
- cvat/apps/webhooks/migrations/0003_alter_webhookdelivery_status_code.py
- cvat/apps/lambda_manager/permissions.py
- cvat/apps/iam/serializers.py
- cvat/apps/events/const.py
- cvat/apps/webhooks/permissions.py
- cvat/apps/iam/filters.py
- cvat/apps/organizations/views.py
- cvat/apps/organizations/rules/tests/generators/organizations_test.gen.rego.py
- cvat/apps/lambda_manager/rules/tests/generators/lambda_test.gen.rego.py
- cvat/apps/lambda_manager/views.py
- cvat/apps/organizations/permissions.py
- cvat/apps/lambda_manager/urls.py
- cvat/apps/events/rules/tests/generators/events_test.gen.rego.py
- cvat/apps/events/apps.py
- cvat/apps/events/export.py
- cvat/apps/log_viewer/permissions.py
- cvat/apps/iam/middleware.py
- cvat/apps/webhooks/serializers.py
- cvat/apps/organizations/migrations/0001_initial.py
- cvat/apps/events/utils.py
- cvat/apps/log_viewer/rules/tests/generators/analytics_test.gen.rego.py
- cvat/apps/iam/urls.py
- cvat/apps/webhooks/models.py
- cvat/apps/lambda_manager/tests/test_lambda.py
- cvat/apps/organizations/throttle.py
- cvat/apps/iam/tests/test_rest_api.py
- cvat/apps/events/views.py
- cvat/apps/health/management/commands/workerprobe.py
- cvat/apps/events/cache.py
- cvat/apps/webhooks/views.py
- cvat/apps/events/handlers.py
- cvat/apps/webhooks/migrations/0004_alter_webhook_target_url.py
- cvat/apps/organizations/models.py
- cvat/apps/iam/schema.py
- cvat/apps/iam/permissions.py
- cvat/apps/organizations/admin.py
- cvat/apps/webhooks/event_type.py
- cvat/apps/organizations/serializers.py
- cvat/apps/dataset_repo/migrations/0006_gitdata_format.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: testing
- GitHub Check: generate_github_pages
- GitHub Check: Analyze (python)
- GitHub Check: Linter
- GitHub Check: Analyze (javascript)
🔇 Additional comments (15)
cvat/apps/organizations/rules/tests/generators/memberships_test.gen.rego.py (2)
101-108: String literal formatting changes look good.The conversion from single to double quotes aligns with the PR's objective to standardize string formatting across the codebase.
121-129: Nice improvement in dictionary structure readability.The restructuring of the conditional dictionary assignment improves code readability while maintaining the same functionality. The new format with proper indentation makes the code structure clearer and more maintainable.
cvat/apps/organizations/rules/tests/generators/invitations_test.gen.rego.py (1)
112-120: Consistent formatting improvement.The restructuring of the dictionary assignment matches the improvements made in memberships_test.gen.rego.py, maintaining consistency across the codebase while improving readability.
cvat/apps/iam/apps.py (1)
10-15: LGTM! Clean implementation following Django best practices.The changes align with the PR objectives for code formatting, and the signal registration in
ready()follows Django's recommended practices for app initialization.cvat/apps/log_viewer/apps.py (1)
Line range hint
9-15: LGTM! Good addition of type hints.The changes maintain consistency with other apps while improving type safety through the addition of return type annotation.
cvat/apps/health/apps.py (1)
10-15: LGTM! Verify plugin registration impact.The changes look good and follow Django's plugin system practices. However, since we're registering a health check plugin, we should verify its impact.
Let's check for any potential plugin registration conflicts:
✅ Verification successful
Plugin registration verified - no conflicts found
The OPAHealthCheck plugin is properly registered and implemented:
- Single registration point in apps.py
- Correctly inherits from BaseHealthCheckBackend
- No conflicting health check implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other OPAHealthCheck registrations to avoid duplicates rg "plugin_dir.register.*OPAHealthCheck" --type pyLength of output: 121
Script:
#!/bin/bash # Check OPAHealthCheck implementation and other health checks echo "=== OPAHealthCheck Implementation ===" ast-grep --pattern 'class OPAHealthCheck { $$$ }' echo -e "\n=== Other Health Check Plugins ===" rg "class.*HealthCheck" --type pyLength of output: 334
cvat/apps/organizations/apps.py (1)
10-15: LGTM! Consistent with other app configurations.The changes maintain the same high-quality pattern seen in other apps, with proper type hints and permission loading.
cvat/apps/lambda_manager/apps.py (1)
Line range hint
12-16: Verify the functional change: Addition of permission loadingThis change appears to be a functional modification rather than just formatting. While the formatting changes align with the PR's objective, the addition of permission loading represents new behavior.
✅ Verification successful
Permission loading is consistent with codebase patterns
The addition of permission loading in
lambda_managerapp aligns with the established pattern across all other Django apps in the system. This change appears to be fixing a missing permission registration to maintain consistency with the rest of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this pattern is consistently applied across other apps rg -l "load_app_permissions\(self\)" "cvat/apps/**/apps.py"Length of output: 122
Script:
#!/bin/bash # First, find all apps.py files and check for permission loading fd "apps.py$" | xargs rg "load_app_permissions\(self\)" -l # Also check the import pattern to understand the scope fd "apps.py$" | xargs rg "from cvat.apps.iam.permissions import load_app_permissions" -lLength of output: 631
cvat/apps/webhooks/apps.py (1)
11-15: Consistent pattern: Permission loading addedThe addition of permission loading follows the same pattern seen in other app configurations. This appears to be a systematic change across multiple apps.
cvat/apps/iam/rules/tests/generate_tests.py (1)
Line range hint
18-28: Well-structured argument parser implementationGood separation of concerns by extracting the argument parser creation into a dedicated function. The default path is correctly derived from REPO_ROOT.
cvat/apps/lambda_manager/serializers.py (2)
13-21: Well-structured label mapping serializerGood implementation of the new serializer with clear field definitions and helpful documentation.
37-42: Good deprecation handlingAppropriate handling of the deprecated field with:
- Clear deprecation notice in help text
- Proper source mapping to new field
- Maintaining backward compatibility
cvat/apps/webhooks/signals.py (3)
204-204: LGTM! Clean dictionary comprehension.The dictionary comprehension improves code readability while maintaining functionality.
255-259: LGTM! Improved list comprehension readability.The list comprehension is now more readable with proper line breaks and indentation.
67-67: Verify response size limit handling.The response size limit is enforced, but the code continues reading the response even after exceeding the limit. Consider breaking early to avoid unnecessary data transfer.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8902 +/- ##
========================================
Coverage 73.93% 73.93%
========================================
Files 411 411
Lines 44223 44236 +13
Branches 3993 3993
========================================
+ Hits 32695 32705 +10
- Misses 11528 11531 +3
|
Motivation and context
These apps have either no or very few PRs that update them, so this seems like a good time to reformat them.
How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Code Quality Improvements
New Functionality
load_app_permissions()calls in app initialization methodsMinor Enhancements
These changes primarily focus on code quality and maintainability without introducing significant user-facing changes.