142 lines
6.4 KiB
Markdown
142 lines
6.4 KiB
Markdown
# Python/FastAPI Code Review Checklist
|
|
|
|
Use this checklist when reviewing Python/FastAPI code or creating pull requests for Grey Haven projects.
|
|
|
|
## Formatting & Style
|
|
|
|
- [ ] **Line length**: Code lines do not exceed 130 characters (CRITICAL!)
|
|
- [ ] **Indentation**: Uses 4 spaces (not tabs or 2 spaces)
|
|
- [ ] **Quotes**: Uses double quotes for strings (Ruff default)
|
|
- [ ] **Line endings**: Uses Unix-style line endings (\\n)
|
|
- [ ] **Ruff formatted**: Code is formatted with Ruff (`ruff format .`)
|
|
- [ ] **Ruff linting**: No Ruff linting errors (`ruff check .`)
|
|
|
|
## Type Hints
|
|
|
|
- [ ] **Function signatures**: ALL functions have type hints (CRITICAL!)
|
|
- Parameters: `def get_user(user_id: str) -> Optional[User]:`
|
|
- Return types: Always include return type annotation
|
|
- [ ] **MyPy passing**: `mypy app/` has no type errors
|
|
- [ ] **Optional types**: Uses `Optional[T]` or `T | None` for nullable values
|
|
- [ ] **Generic types**: Uses proper generic types (`list[str]`, `dict[str, Any]`)
|
|
- [ ] **Type imports**: Imports types from `typing` module
|
|
|
|
## Database Models (SQLModel)
|
|
|
|
- [ ] **snake_case fields**: ALL database column names use snake_case (CRITICAL!)
|
|
- ✅ `created_at`, `tenant_id`, `email_address`, `is_active`
|
|
- ❌ `createdAt`, `tenantId`, `emailAddress`, `isActive`
|
|
- [ ] **Multi-tenant**: Models include `tenant_id` field
|
|
- [ ] **Field descriptions**: All fields have `description` parameter
|
|
- [ ] **Indexes**: Frequently queried fields have `index=True`
|
|
- [ ] **Constraints**: Foreign keys, unique constraints properly defined
|
|
- [ ] **Timestamps**: Uses UTC datetime (`UTCDateTime` type if available)
|
|
- [ ] **Table names**: Uses `__tablename__` with lowercase plural names
|
|
|
|
## Pydantic Schemas
|
|
|
|
- [ ] **Schema hierarchy**: Follows Base/Create/Update/Response pattern
|
|
- [ ] **Validators**: Uses `@field_validator` for custom validation
|
|
- [ ] **ConfigDict**: Response schemas have `model_config = ConfigDict(from_attributes=True)`
|
|
- [ ] **Field constraints**: Uses appropriate constraints (`ge`, `le`, `max_length`, etc.)
|
|
- [ ] **Optional fields**: Update schemas use `| None` for optional fields
|
|
- [ ] **Descriptions**: All fields have descriptions in `Field(..., description="...")`
|
|
|
|
## FastAPI Endpoints
|
|
|
|
- [ ] **Type hints**: ALL endpoint functions fully typed with `Annotated`
|
|
- [ ] **Docstrings**: Endpoints have comprehensive docstrings (Args, Returns, Raises)
|
|
- [ ] **Status codes**: Uses appropriate HTTP status codes from `status` module
|
|
- [ ] **Response models**: Endpoints specify `response_model`
|
|
- [ ] **Dependencies**: Uses Depends() for repository and auth dependencies
|
|
- [ ] **Error handling**: Raises HTTPException with proper status codes and messages
|
|
- [ ] **Router prefix**: Router has appropriate prefix and tags
|
|
|
|
## Repository Pattern
|
|
|
|
- [ ] **Tenant isolation**: ALL queries filter by `tenant_id` (CRITICAL!)
|
|
- [ ] **Type hints**: Repository methods fully typed
|
|
- [ ] **Async/await**: Uses async/await for database operations
|
|
- [ ] **Session management**: Properly commits and refreshes after changes
|
|
- [ ] **Error handling**: Handles database errors appropriately
|
|
- [ ] **CRUD methods**: Implements standard create, read, update, delete methods
|
|
|
|
## Multi-Tenant Architecture
|
|
|
|
- [ ] **Tenant filtering**: All queries include tenant_id filter
|
|
- [ ] **Repository methods**: Accept `tenant_id` parameter
|
|
- [ ] **Validation**: Validates user has access to requested tenant
|
|
- [ ] **Isolation**: No cross-tenant data leakage possible
|
|
- [ ] **Foreign keys**: Multi-tenant relationships properly enforced
|
|
|
|
## Imports Organization
|
|
|
|
- [ ] **Import order**: Follows Ruff isort rules:
|
|
1. Standard library imports
|
|
2. Third-party imports
|
|
3. Local imports (app.*)
|
|
- [ ] **Absolute imports**: Uses absolute imports (not relative)
|
|
- [ ] **Grouped imports**: Related imports grouped together
|
|
- [ ] **No unused imports**: All imports are used
|
|
|
|
## Testing (Pytest)
|
|
|
|
- [ ] **Tests exist**: Endpoints/functions have corresponding tests
|
|
- [ ] **Test markers**: Uses pytest markers (@pytest.mark.unit, @pytest.mark.integration)
|
|
- [ ] **Fixtures**: Uses pytest fixtures for setup
|
|
- [ ] **Async tests**: Async tests decorated with `@pytest.mark.asyncio`
|
|
- [ ] **Mocking**: Uses AsyncMock/MagicMock for external dependencies
|
|
- [ ] **Coverage**: Maintains or improves test coverage (aim for >80%)
|
|
- [ ] **Assertions**: Tests have clear, specific assertions
|
|
|
|
## Security
|
|
|
|
- [ ] **Input validation**: Uses Pydantic schemas for input validation
|
|
- [ ] **SQL injection**: Uses parameterized queries (SQLModel handles this)
|
|
- [ ] **Authentication**: Endpoints require authentication via dependencies
|
|
- [ ] **Authorization**: Validates user has permission for actions
|
|
- [ ] **Secrets**: Uses environment variables for secrets (never hardcode)
|
|
- [ ] **Rate limiting**: Critical endpoints have rate limiting
|
|
|
|
## Error Handling
|
|
|
|
- [ ] **HTTPException**: Raises HTTPException with proper status codes
|
|
- [ ] **Error messages**: Error messages are descriptive and user-friendly
|
|
- [ ] **Logging**: Errors are logged appropriately
|
|
- [ ] **Validation errors**: Pydantic validation errors return 422
|
|
- [ ] **Not found**: Returns 404 for missing resources
|
|
|
|
## Performance
|
|
|
|
- [ ] **Database queries**: Queries are efficient (no N+1 problems)
|
|
- [ ] **Indexes**: Frequently filtered columns have indexes
|
|
- [ ] **Pagination**: List endpoints implement pagination (limit/offset)
|
|
- [ ] **Async operations**: Uses async/await for I/O operations
|
|
- [ ] **Connection pooling**: Database uses connection pooling
|
|
|
|
## Documentation
|
|
|
|
- [ ] **Docstrings**: All functions have comprehensive docstrings
|
|
- [ ] **OpenAPI docs**: FastAPI auto-docs are accurate and complete
|
|
- [ ] **Type annotations**: Type hints serve as documentation
|
|
- [ ] **README updated**: README reflects any new features/changes
|
|
- [ ] **API examples**: Complex endpoints have usage examples
|
|
|
|
## Pre-commit Checks
|
|
|
|
- [ ] **Virtual env active**: Ran commands with `source .venv/bin/activate`
|
|
- [ ] **Ruff formatting**: `ruff format .` applied
|
|
- [ ] **Ruff linting**: `ruff check --fix .` passing
|
|
- [ ] **MyPy**: `mypy app/` passing with no type errors
|
|
- [ ] **Tests passing**: `pytest` passes all tests
|
|
- [ ] **Coverage**: Test coverage meets threshold (>80%)
|
|
- [ ] **Pre-commit hooks**: All pre-commit hooks pass
|
|
|
|
## API Design
|
|
|
|
- [ ] **RESTful**: Endpoints follow REST principles
|
|
- [ ] **Naming**: Endpoints use clear, descriptive names
|
|
- [ ] **Versioning**: API versioned appropriately (`/v1/`)
|
|
- [ ] **Consistency**: Similar endpoints have consistent patterns
|
|
- [ ] **CRUD complete**: Resource has full CRUD operations if needed
|