Skip to main content

Code Review Guidelines

This document outlines the code review process and standards for the Serko Northsky project.

Review Process​

  1. Create Pull Request — All changes go through PRs
  2. Automated Checks — CI runs tests, linting, type checking
  3. Peer Review — At least one approval required
  4. Address Feedback — Resolve all comments
  5. Merge — Squash merge to main branch

Review Checklist​

Architecture​

  • Follows layered architecture (API → Service → Repository)
  • Uses dependency injection via containers
  • Respects interface boundaries
  • Uses Unit of Work for transactions

Code Quality​

  • Type hints on all functions and methods
  • No # type: ignore without justification
  • Follows existing code style
  • No magic numbers — use constants
  • Self-documenting variable names

Testing​

  • Tests for new functionality
  • Tests follow naming convention: test_<method>__<when>__<expected>
  • Fixtures used appropriately
  • No test interdependencies

Documentation​

  • Docstrings for public classes and methods
  • Comments for complex logic
  • README updated if needed

Python Standards​

Imports​

# ✅ Good: Module imports at top
from __future__ import annotations
from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from serko_northsky.models.database_entities import User

# ❌ Bad: Import inside function (except for circular imports)
def get_user():
    from serko_northsky.models.database_entities import User

Type Hints​

# ✅ Good: Full type hints with Python 3.13 syntax
def create_user(email: str, name: str | None = None) -> User:
    ...

async def get_users() -> list[User]:
    ...

# ❌ Bad: Missing hints or using old syntax
def create_user(email, name=None):
    ...

Unit of Work Usage​

# ✅ Good: Proper UoW usage
class UserService(BaseUserService):
    def __init__(self, uow: BaseUnitOfWork) -> None:
        self.uow = uow

    async def create(self, email: str) -> User:
        user = User(email=email)
        created = await self.uow.users.create(user)
        await self.uow.save_changes()  # Explicit commit
        return created

# ❌ Bad: Direct session access
async def create(self, email: str) -> User:
    user = User(email=email)
    self.uow.session.add(user)
    await self.uow.session.commit()  # Don't do this!

Test Naming​

# ✅ Good: Descriptive test names
class TestUserService:
    async def test_create__when_email_valid__returns_user(self):
        ...

    async def test_create__when_email_exists__raises_error(self):
        ...

# ❌ Bad: Vague test names
def test_create(self):
    ...

def test_user(self):
    ...

Review Comments​

Be Constructive​

# ✅ Good
Consider using `get_by_field` here instead of raw query for consistency:
```python
user = await self.uow.users.get_by_field("email", email)

❌ Bad

This is wrong. Use get_by_field.


### Be Specific

```markdown
# ✅ Good
This will fail for empty lists. Add a guard:
```python
if not items:
    return []

❌ Bad

Handle edge cases.


### Ask Questions

```markdown
# ✅ Good
I'm not sure I understand the intent here. Could you explain why we're 
checking `is_active` before `is_verified`? Is there a specific ordering 
requirement?

# ❌ Bad
Why?

Approval Criteria​

Must Pass​

  • All CI checks pass
  • No unresolved blocking comments
  • At least one approval

Should Have​

  • Test coverage maintained or improved
  • No new linting warnings
  • Documentation updated

Nice to Have​

  • Performance considerations documented
  • Migration path for breaking changes

Common Patterns​

Service Pattern​

class MyService(BaseMyService):
    def __init__(self, uow: BaseUnitOfWork) -> None:
        self.uow = uow

    async def my_method(self, arg: str) -> Result:
        # 1. Validate input
        # 2. Business logic
        # 3. Persist changes
        await self.uow.save_changes()
        return result

Router Pattern​

@router.post("/", status_code=status.HTTP_201_CREATED)
async def create_item(
    request: CreateItemRequest,
    service: Annotated[BaseItemService, Depends(dependencies.get_item_service)],
    _auth: Annotated[dict[str, Any], Depends(require_auth)],
) -> ItemResponse:
    item = await service.create(request)
    return ItemResponse.model_validate(item)