На что обратишь внимание при получении pull request?

На что обращать внимание при review pull request

При рассмотрении PR я проверяю несколько категорий — от архитектуры до стиля кода. Это систематический подход, который помогает поймать проблемы на ранней стадии.

1. Требования и контекст

Вначале я разбираюсь с целью PR:

- Что именно решает этот PR? (прочитать описание)
- Какой тикет или issue он закрывает?
- Есть ли links к документации или спеку?
- Соответствует ли код описанию?

Красные флаги:

• PR без описания или с размытым описанием
• Изменения, которые не соответствуют title
• Слишком большие PR (>500 строк) — просить разделить

2. Архитектура и дизайн

Проверяю, соответствует ли решение архитектурным принципам:

# ❌ Плохо — нарушение слоёв (presentation знает о domain)
class UserView:
    def get(self, request):
        user = User.objects.get(id=request.user.id)  # Прямой доступ к БД!
        user.recalculate_score()  # Бизнес-логика в view
        return JsonResponse(user.to_dict())

# ✅ Хорошо — правильное разделение ответственности
class UserView:
    def get(self, request):
        use_case = GetUserUseCase()
        user_dto = use_case.execute(request.user.id)
        return JsonResponse(user_dto.to_dict())

class GetUserUseCase:  # application layer
    def execute(self, user_id):
        repo = UserRepository()
        user = repo.get_by_id(user_id)
        user.recalculate_score()
        return UserDTO.from_entity(user)

Проверяю:

• Соответствие Clean Architecture (domain → application → infrastructure → presentation)
• Dependency Injection вместо прямого импорта
• Нарушение Single Responsibility
• Неправильные абстракции (OOP vs functional)

3. Обработка ошибок

# ❌ Плохо — молчаливое игнорирование
def process_data():
    try:
        data = fetch_from_api()
        return parse(data)
    except:
        pass  # Что случилось??

# ❌ Плохо — двойное логирование
def process_data():
    try:
        data = fetch_from_api()
    except RequestError as e:
        logger.error(f"Failed to fetch: {e}")  # Логируем
        raise  # Потом логируем на уровне выше (двойное логирование)

# ✅ Хорошо — явная обработка, одна точка логирования
def process_data():
    """Fetch and parse data from API"""
    data = fetch_from_api()  # Ошибки обрабатываются на уровне выше
    return parse(data)

def handle_request():
    try:
        result = process_data()
    except (RequestError, ParseError) as e:
        logger.error(f"Processing failed: {e}", extra={"error": str(e)})
        return {"error": "Processing failed"}, 500

Проверяю:

• Нет молчаливых except (кроме явных случаев)
• Нет двойного логирования ошибок
• Правильные типы исключений
• Обработка edge cases

4. Безопасность

# ❌ SQL injection
def get_user(user_id):
    query = f"SELECT * FROM users WHERE id = {user_id}"
    return db.execute(query)

# ❌ Хардкодированные секреты
API_KEY = "sk-1234567890abcdef"

# ❌ Отсутствие валидации
@app.post("/users")
def create_user(data):
    user = User(email=data["email"])  # Кто проверил email?

# ✅ Правильно — параметризованные запросы
def get_user(user_id: int):
    return db.query(User).filter(User.id == user_id).first()

# ✅ Переменные окружения
API_KEY = os.getenv("API_KEY")

# ✅ Валидация
from pydantic import BaseModel, EmailStr

class CreateUserRequest(BaseModel):
    email: EmailStr  # Автоматическая валидация
    name: str

Проверяю:

• Нет SQL injection (параметризованные запросы)
• Нет хардкодированных токенов/паролей
• Валидация input данных
• Правильные CORS, CSRF защиты
• Нет утечки sensitive информации в логах

5. Тестирование

# ❌ Плохо — нет тестов
class UserRepository:
    def get_active_users(self):
        return User.objects.filter(is_active=True)  # Не протестировано

# ❌ Плохо — тест на реальных данных
def test_user_creation():
    user = User.objects.create(email="test@example.com")  # ОРМ!
    assert user.email == "test@example.com"

# ✅ Хорошо — unit тесты с мокированием
def test_get_active_users():
    # Arrange
    mock_user_1 = Mock(spec=User, is_active=True)
    mock_user_2 = Mock(spec=User, is_active=False)
    mock_repo = Mock(spec=UserRepository)
    mock_repo.query.return_value.filter.return_value = [mock_user_1]
    
    # Act
    result = mock_repo.get_active_users()
    
    # Assert
    assert len(result) == 1
    assert result[0].is_active is True

# ✅ Правильно — тесты для критичного функционала
def test_discount_calculation():
    # Arrange
    user = UserFactory(subscription_level="premium")
    cart = CartFactory(total=1000)
    
    # Act
    discount = calculate_discount(user, cart)
    
    # Assert
    assert discount == 100  # 10% скидка для premium
    assert discount_reason == "PREMIUM_SUBSCRIPTION"

Проверяю:

• Есть ли тесты для нового функционала?
• Покрытие >= 80% для важного кода
• Тесты используют моки, не реальные данные
• Тесты быстрые (< 100ms за тест)
• Нет flaky тестов (не случайно падают)

6. Производительность

# ❌ N+1 запрос к БД
for user in users:  # SELECT * FROM users
    print(user.profile.bio)  # SELECT * FROM profiles WHERE user_id = ?
                              # Повторяется для каждого пользователя!

# ✅ Правильно — eager loading
users = User.objects.prefetch_related(profile).all()
for user in users:
    print(user.profile.bio)  # Один доп. запрос для всех профилей

# ❌ Плохо — обработка миллионов записей в памяти
def export_users_to_csv():
    users = User.objects.all()  # 1 миллион пользователей в памяти!
    return write_csv(users)

# ✅ Хорошо — потоковая обработка
def export_users_to_csv():
    users = User.objects.all().iterator(chunk_size=1000)
    for chunk in users:
        write_csv_chunk(chunk)

Проверяю:

• Нет N+1 запросов к БД
• Нет загрузки миллионов записей в памяти
• Кэширование повторяющихся операций
• Асинхронность для I/O операций

7. Стиль кода

# ❌ Плохо
def f(u):
    return [u["name"] if u.get("name") else "Unknown" for u in u]

# ✅ Хорошо
def get_user_names(users: list[dict]) -> list[str]:
    """Extract user names, defaulting to Unknown if missing."""
    return [
        user.get("name", "Unknown")
        for user in users
    ]

# ❌ Плохо — магические числа
if age > 65:
    discount = 0.25

# ✅ Хорошо
RETIREMENT_AGE = 65
RETIREMENT_DISCOUNT = 0.25

if age > RETIREMENT_AGE:
    discount = RETIREMENT_DISCOUNT

Проверяю:

• Соответствие PEP8 (используется ruff/black)
• Понятные имена переменных
• Нет магических чисел
• Docstrings для публичных функций
• Нет избыточной сложности

8. Миграции и БД

# ❌ Плохо — миграция без тестирования
class Migration(migrations.Migration):
    operations = [
        migrations.AddField(
            model_name=user,
            name=new_field,
            field=models.CharField(max_length=100),
        ),
    ]

# ✅ Хорошо — миграция с тестированием
# 1. Создали миграцию
# 2. Откатили её: ./manage.py migrate --zero
# 3. Накатили обратно: ./manage.py migrate
# 4. Проверили работу

Проверяю:

• Миграции написаны и протестированы
• Обратная совместимость (backward compatibility)
• Нет потери данных

9. Документация

# ✅ Документация для сложного алгоритма
def calculate_ranking_score(user_stats: UserStats) -> float:
    """
    Calculate user ranking score using multi-factor algorithm.
    
    Factors:
    - Activity: 40% (posts, comments, likes)
    - Quality: 35% (avg rating, helpful marks)
    - Consistency: 25% (days active, streak)
    
    Args:
        user_stats: User activity statistics
    
    Returns:
        Score between 0.0 and 100.0
    
    Examples:
        >>> stats = UserStats(posts=10, avg_rating=4.5)
        >>> calculate_ranking_score(stats)
        72.5
    """
    ...

Проверяю:

• Сложная логика задокументирована
• API endpoints имеют описание
• CHANGELOG обновлён (если нужно)

10. Dependencies

# ❌ Плохо — добавлены ненужные зависимости
requirements.txt:
    requests==2.31.0
    numpy==1.24.0  # Не используется
    pandas==2.0.0  # Не используется

# ✅ Хорошо — только необходимые
requirements.txt:
    requests==2.31.0
    pydantic==2.0.0

Проверяю:

• Нет неиспользуемых импортов и зависимостей
• Версии зависимостей обоснованы
• Нет уязвимостей (проверить через safety)

Чеклист для review

• Понимаю, что решает PR
• Код соответствует архитектуре (Clean Architecture)
• Нет критичных ошибок безопасности
• Есть тесты (coverage >= 80%)
• Нет N+1 запросов
• Стиль кода соответствует стандартам
• Нет дублирования кода
• Миграции протестированы
• Не добавлены ненужные зависимости
• Документация обновлена

Главный принцип: я ищу не синтаксические ошибки (их должны ловить linter и тесты), а логические ошибки и архитектурные проблемы, которые приведут к проблемам в production.