pythonista-reviewing

name	pythonista-reviewing
description	Code review best practices for Python projects. Use when reviewing code, PRs, or diffs. Triggers on "review", "code review", "PR", "pull request", "diff", "check this", "look at this code", "quality", "refactor", or when examining code for issues.

Code Review Best Practices

Core Philosophy

Review at PR level, not file-by-file. Focus on egregious structural issues, not minutia. Look for cross-file patterns that indicate architectural problems.

Quick Start

Option 1: LLM-Based Whole-PR Review (Fast, High-Level)

Use the llm CLI tool for fast whole-PR reviews with large context models:

# Extract diff and review with Claude Sonnet
./scripts/extract-diff.sh && ./scripts/review-with-llm.sh

# Use different models for different perspectives
./scripts/review-with-llm.sh -m opus       # Deeper analysis
./scripts/review-with-llm.sh -m gpt-4-turbo  # Different insights

# Include project guidelines
./scripts/review-with-llm.sh -g docs/guidelines/

Prerequisites: Install Simon Willison's llm tool:

pip install llm && llm keys set anthropic

See scripts/README.md for full setup instructions.

Option 2: Manual Diff Extraction

# ALWAYS filter out lock files first
git diff main...HEAD -- . ':!uv.lock' ':!poetry.lock' > /tmp/pr-diff.txt
wc -lc /tmp/pr-diff.txt  # Most diffs fit after this

# If still too big, filter more
git diff main...HEAD -- . ':!uv.lock' ':!docs/*' > /tmp/code-diff.txt

What to Look For - Egregious Issues Only

Checklist

• Code duplication - Same logic in 2+ places?
• Repeated patterns - Same code structure 3+ times?
• God functions - Functions over 100 lines?
• Weak types - Any, object, raw dict/list, missing annotations?
• Type-deficient patterns - hasattr, getattr instead of proper types?
• Meaningless tests - Tests that just verify assignment?
• Dead code - Unused functions, commented code?

What NOT to Flag

• Variable naming (unless truly confusing)
• Line length
• Comment style
• Whitespace
• Import order

These are auto-fixable or minor. Focus on structural problems.

Egregious Issues

1. Massive Code Duplication

# WRONG - Same validation in two files
# file1.py
def process_user_data(user):
    if not user.get("email"):
        raise ValueError("Email required")
    if not user.get("name"):
        raise ValueError("Name required")

# file2.py - DUPLICATE!
def validate_user(user):
    if not user.get("email"):
        raise ValueError("Email required")
    if not user.get("name"):
        raise ValueError("Name required")

# CORRECT - Extract once, reuse
def validate_user_fields(user: User) -> User:
    if not user.email:
        raise ValueError("Email required")
    if not user.name:
        raise ValueError("Name required")
    return user

2. God Functions (100+ lines)

# WRONG - 200 line function doing everything
def process_order(order_data):
    # 50 lines of validation
    # 30 lines of transformation
    # 40 lines of API calls
    # 30 lines of database updates
    # 50 lines of error handling
    ...

# CORRECT - Extract responsibilities
def process_order(order_data: OrderData) -> ProcessedOrder:
    validated = _validate_order(order_data)
    transformed = _transform_order(validated)
    api_result = _call_payment_api(transformed)
    return _save_order(transformed, api_result)

3. Weak Types

# WRONG - Any, raw dict/list
def process_data(data: Any) -> dict:
    return data.get("value")

def get_users() -> list:  # List of what?
    return [{"name": "Bob"}]

# CORRECT - Specific types
def process_data(data: UserData) -> UserResponse:
    return UserResponse(value=data.value)

def get_users() -> list[User]:
    return [User(name="Bob")]

4. hasattr/getattr (Type Deficiency Markers)

# WRONG - Using hasattr because type is wrong
def process(obj: Any):
    if hasattr(obj, "name"):
        return obj.name
    return None

# CORRECT - Use Protocol
class Named(Protocol):
    name: str

def process(obj: Named) -> str:
    return obj.name

5. Related Classes Without Shared Interface

# WRONG - Related classes with drifting interfaces
class CacheDirectory:
    def list_entries(self, filter=None): ...
    def count(self, filter=None): ...

class StatusDirectory:
    def get_all_entries(self): ...  # Different name!
    def total_count(self): ...  # Different name!

# CORRECT - ABC enforces consistent interface
class FileDirectoryBase(ABC, Generic[T]):
    @abstractmethod
    def list_entries(self) -> list[T]: ...
    @abstractmethod
    def count(self) -> int: ...

6. Meaningless Tests

# WRONG - Tests assignment
def test_user_created():
    user = User(name="Bob", email="[e-mail]")
    assert user is not None  # Meaningless
    assert user.name == "Bob"  # Just testing assignment

# CORRECT - Tests invariants
def test_user_email_must_be_valid():
    """INVARIANT: User email must contain @ and domain."""
    with pytest.raises(ValidationError):
        User(name="Bob", email="invalid-email")

After Code Review

CRITICAL: After completing a code review, ALWAYS:

1. Present findings - Output the full review report
2. List suggested actions - Number each potential fix
3. Ask for approval - Before creating TODOs or executing
4. Wait for explicit approval - User may reject or modify

Example flow:

[Code review findings output]

## Suggested Actions
1. Add format-duration validator to ClipV4PromptSchema
2. Add tests for format-duration validation
3. Move CLIPS_PER_HOUR to config class

Which items should I proceed with?

Scripts

The plugin includes scripts for LLM-powered code review:

• scripts/extract-diff.sh - Extract diffs in PR, commit, or path mode
• scripts/review-with-llm.sh - Run code review with llm CLI tool
• scripts/README.md - Setup instructions and usage examples

Reference Files

• references/what-to-flag.md - Detailed patterns to flag

Related Skills

• For pattern discovery, see /pythonista-patterning
• For testing patterns, see /pythonista-testing
• For type safety, see /pythonista-typing