feat: add basedpyright type checks #15
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new GitHub Actions workflow for BasedPyright static type checking was introduced. The Black formatter and test workflows were updated to specify repository permissions. The Black formatter job was renamed. The Changes
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🔭 Outside diff range comments (1)
.github/workflows/basedpywight_type_check.yml (1)
1-30: Fix the critical filename typo.The workflow implementation is solid, but there's a typo in the filename that needs to be corrected.
The filename should be
basedpyright_type_check.ymlnotbasedpywight_type_check.yml. This typo will cause confusion and inconsistency with the actual tool name.Workflow structure looks good:
- Appropriate permissions with
contents: read- Proper Python 3.13 setup with Poetry caching
- Correct dependency installation with
--no-root --with dev- Appropriate type checking command with
--level error
🧹 Nitpick comments (2)
src/curator/harbor.py (2)
85-86: Consider using proper logging instead of print statements.The error handling improvement adds useful logging, but using
print()in production code is not ideal.Consider using Python's logging module for better log management:
+import logging + +logger = logging.getLogger(__name__) + # In the exception handler: - message = f"Failed to fetch from Harbor: {str(e)}" - print(message) + message = f"Failed to fetch from Harbor: {str(e)}" + logger.error(message)Also applies to: 89-89
92-93: Apply consistent logging approach.Same logging improvement recommendation applies to the generic exception handler.
Use the logging module here as well for consistency:
- message = f"An error occurred while processing Harbor data: {str(e)}" - print(message) + message = f"An error occurred while processing Harbor data: {str(e)}" + logger.error(message)Also applies to: 96-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/basedpywight_type_check.yml(1 hunks).github/workflows/black_formatter_check.yml(2 hunks).github/workflows/tests.yml(1 hunks)pyproject.toml(1 hunks)src/curator/harbor.py(2 hunks)
🔇 Additional comments (4)
.github/workflows/tests.yml (1)
6-7: LGTM! Security improvement with explicit permissions.Adding explicit
contents: readpermission follows the principle of least privilege and improves workflow security..github/workflows/black_formatter_check.yml (3)
6-7: LGTM! Consistent security improvement.Adding explicit permissions aligns with the security improvements made across all workflows.
10-10: Good job name standardization.The job name change to
format-checkimproves consistency with the newtype-checkjob naming convention.
26-26: Consistent dependency installation approach.The
--no-root --with devflags align with the type checking workflow and are appropriate for CI environments.
| try: | ||
| latest_artifact = await get_latest_artifact() | ||
|
|
||
| assert latest_artifact.extra_attrs |
There was a problem hiding this comment.
Replace assertion with proper runtime validation.
Using assert for runtime validation in production code is problematic since assertions can be disabled with the -O flag, potentially causing runtime errors.
Replace the assertion with proper runtime validation:
- assert latest_artifact.extra_attrs
+ if not latest_artifact.extra_attrs:
+ raise HTTPException(
+ status_code=404,
+ detail="No extra attributes found in the latest artifact"
+ )📝 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.
| assert latest_artifact.extra_attrs | |
| if not latest_artifact.extra_attrs: | |
| raise HTTPException( | |
| status_code=404, | |
| detail="No extra attributes found in the latest artifact" | |
| ) |
🤖 Prompt for AI Agents
In src/curator/harbor.py at line 67, replace the assert statement used for
runtime validation with an explicit check that raises an appropriate exception
if latest_artifact.extra_attrs is missing or falsy. This ensures the validation
always runs regardless of Python optimization flags and prevents silent failures
in production.
No description provided.