Skip to content

pr-guidelines.md

Latest commit

 

History

History
498 lines (387 loc) · 15.4 KB

pr-guidelines.md

File metadata and controls

498 lines (387 loc) · 15.4 KB

Pull Request Guidelines

Руководство по созданию и review Pull Requests для курса.

Содержание

Создание PR

1. Перед началом работы

# Обновите main ветку
git checkout main
git pull origin main

# Создайте feature branch
git checkout -b feature/user-authentication

# Или для фиксов
git checkout -b fix/order-calculation

2. Работа над задачей

# Делайте коммиты часто и атомарно
git add src/auth/handlers.py
git commit -m "feat(auth): add login endpoint"

git add tests/test_auth.py
git commit -m "test(auth): add tests for login endpoint"

# Пушьте ветку
git push origin feature/user-authentication

3. Создание Pull Request

На GitHub:

  1. Перейдите в репозиторий
  2. Нажмите "New Pull Request"
  3. Выберите вашу ветку
  4. Заполните template (см. ниже)
  5. Добавьте reviewers
  6. Создайте PR

Требования к PR:

  • ✅ Понятное название
  • ✅ Описание изменений
  • ✅ Ссылка на задачу (если есть)
  • ✅ Скриншоты (для UI изменений)
  • ✅ Прохождение тестов в CI
  • ✅ Нет конфликтов с main

PR Template

Создайте файл .github/pull_request_template.md:

## Описание
Кратко опишите, что делает этот PR.

## Тип изменений
- [ ] Новая функциональность (feat)
- [ ] Исправление бага (fix)
- [ ] Рефакторинг (refactor)
- [ ] Документация (docs)
- [ ] Тесты (test)
- [ ] CI/CD (chore)

## Что было сделано
- Добавлен endpoint для регистрации пользователей
- Реализована валидация email
- Добавлены unit тесты

## Как тестировать
1. Запустить приложение: `python main.py`
2. Отправить POST запрос на `/api/v1/auth/register`
3. Проверить, что пользователь создан

## Checklist
- [ ] Код соответствует [Code Style Guide](../docs/code-style-guide.md)
- [ ] Добавлены/обновлены тесты
- [ ] Все тесты проходят локально
- [ ] Добавлена/обновлена документация
- [ ] Проверен в IDE на отсутствие warnings
- [ ] Проведен self-review
- [ ] Нет закомментированного кода
- [ ] Нет debug prints

## Связанные задачи
Closes #123
Fixes #456
Related to #789

## Скриншоты (если применимо)
![Screenshot](link-to-image)

## Дополнительные комментарии
Любая дополнительная информация для reviewers.

Code Review

Для автора PR

1. Self-review перед созданием PR:

# Проверьте diff
git diff main...your-branch

# Запустите тесты
pytest

# Проверьте линтеры
ruff check .
mypy .

# Проверьте форматирование
black --check .

2. Отвечайте на комментарии:

  • Будьте открыты к критике
  • Объясняйте свои решения
  • Вносите изменения по feedback
  • Помечайте resolved комментарии

3. После approval:

# Обновите ветку перед мержем
git checkout main
git pull origin main
git checkout your-branch
git rebase main

# Исправьте конфликты, если есть
# Затем мержьте через GitHub UI

Для reviewer

1. Что проверять:

Функциональность

  • ✅ Код решает поставленную задачу
  • ✅ Нет очевидных багов
  • ✅ Edge cases обработаны
  • ✅ Ошибки обрабатываются правильно

Код качество

  • ✅ Следует Code Style Guide
  • ✅ DRY принцип соблюден
  • ✅ Функции и переменные названы понятно
  • ✅ Нет излишней сложности
  • ✅ Комментарии там, где нужны

Тесты

  • ✅ Покрыты новые функции
  • ✅ Тесты читаемы и понятны
  • ✅ Тесты независимы друг от друга
  • ✅ Тесты проверяют edge cases

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

  • ✅ Нет SQL injection
  • ✅ Input validation присутствует
  • ✅ Секреты не в коде
  • ✅ Правильная аутентификация/авторизация

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

  • ✅ Нет N+1 запросов
  • ✅ Эффективные алгоритмы
  • ✅ Правильное использование индексов
  • ✅ Нет лишних запросов к БД

2. Как комментировать:

# ✅ ХОРОШО: конструктивный комментарий
**Issue:** Этот метод может вызвать N+1 проблему при большом количестве пользователей.

**Suggestion:** Используйте `joinedload` для eager loading связанных данных:
```python
query = select(User).options(joinedload(User.profile))

Why: Это уменьшит количество запросов к БД с N+1 до 1.

❌ ПЛОХО: неконструктивный комментарий

это плохо, переделай


**3. Типы комментариев:**

- 🔴 **CRITICAL** - Должно быть исправлено до merge
- 🟠 **MAJOR** - Желательно исправить
- 🟡 **MINOR** - Необязательно, но было бы хорошо
- 💡 **SUGGESTION** - Идея для улучшения
- ❓ **QUESTION** - Вопрос для понимания
- 👍 **PRAISE** - Похвала за хорошее решение

## Review Checklist

### Python/aiohttp код

```markdown
- [ ] **Handlers** правильно структурированы
  - Используют type hints
  - Валидируют input через Pydantic
  - Обрабатывают ошибки
  - Возвращают правильные HTTP статусы

- [ ] **Database** операции
  - Используют async драйверы
  - Обрабатывают connection errors
  - Правильно закрывают connections
  - Используют transactions где нужно

- [ ] **Тесты**
  - Используют fixtures
  - Изолированы друг от друга
  - Проверяют happy path и edge cases
  - Мокируют внешние зависимости

- [ ] **Безопасность**
  - Пароли хешируются
  - SQL queries параметризованы
  - Input валидируется
  - JWT токены проверяются

- [ ] **Логирование**
  - Используется structlog
  - Логируются важные события
  - Не логируются чувствительные данные
  - Правильный уровень логирования

TypeScript/Express код

- [ ] **Types** правильно определены
  - Все функции типизированы
  - Нет использования `any`
  - Interfaces для объектов
  - Enums для константных значений

- [ ] **Routes** организованы
  - Логика в services, не в routes
  - Middleware используются правильно
  - Error handling есть
  - Валидация input

- [ ] **Database** (если используется)
  - TypeORM entities правильные
  - Миграции созданы
  - Relationships настроены

- [ ] **Тесты**
  - Jest тесты написаны
  - Мокирование зависимостей
  - Покрытие основных сценариев

Docker

- [ ] **Dockerfile**
  - Multi-stage build для production
  - Оптимизирован размер образа
  - .dockerignore настроен
  - Правильный base image

- [ ] **docker-compose.yml**
  - Services правильно связаны
  - Volumes для persistence
  - Environment variables
  - Health checks настроены

Примеры review

Пример 1: Проблема с безопасностью

# ❌ ПЛОХО
@routes.post('/api/v1/users')
async def create_user(request):
    data = await request.json()
    user_id = await db.execute(
        f"INSERT INTO users (username) VALUES ('{data['username']}')"
    )
    return web.json_response({'id': user_id})

Review комментарий:

🔴 CRITICAL: SQL Injection vulnerability

Этот код уязвим к SQL injection атаке. Используйте параметризованные запросы:

@routes.post('/api/v1/users')
async def create_user(request: web.Request) -> web.Response:
    data = await request.json()

    # Валидация через Pydantic
    user_data = UserCreate(**data)

    # Параметризованный запрос
    query = "INSERT INTO users (username) VALUES (:username) RETURNING id"
    user_id = await db.execute(query, {"username": user_data.username})

    return web.json_response({'id': user_id}, status=201)

Пример 2: Проблема с производительностью

# ❌ ПЛОХО
async def get_users_with_orders(request):
    users = await db.fetch_all("SELECT * FROM users")
    result = []
    for user in users:
        orders = await db.fetch_all(
            "SELECT * FROM orders WHERE user_id = :user_id",
            {"user_id": user['id']}
        )
        result.append({'user': user, 'orders': orders})
    return web.json_response(result)

Review комментарий:

🔴 CRITICAL: N+1 Query Problem

При 1000 пользователях это выполнит 1001 запрос к БД. Используйте JOIN или eager loading:

async def get_users_with_orders(request: web.Request) -> web.Response:
    query = """
        SELECT
            u.id, u.username,
            o.id as order_id, o.total
        FROM users u
        LEFT JOIN orders o ON u.id = o.user_id
    """
    rows = await db.fetch_all(query)

    # Group by user
    users_dict = {}
    for row in rows:
        if row['id'] not in users_dict:
            users_dict[row['id']] = {
                'id': row['id'],
                'username': row['username'],
                'orders': []
            }
        if row['order_id']:
            users_dict[row['id']]['orders'].append({
                'id': row['order_id'],
                'total': row['total']
            })

    return web.json_response(list(users_dict.values()))

Пример 3: Хорошая похвала

# ✅ ХОРОШО
async def create_order(request: web.Request) -> web.Response:
    """Создать новый заказ."""
    data = await request.json()
    order_data = OrderCreate(**data)

    async with request.app['db'].transaction():
        # Создать заказ
        order_id = await create_order_record(order_data)

        # Зарезервировать товары
        await reserve_items(order_data.items)

        # Отправить событие
        await publish_event('order.created', {'order_id': order_id})

    return web.json_response({'order_id': order_id}, status=201)

Review комментарий:

👍 PRAISE

Отличное использование транзакции! Если резервирование товаров или отправка события упадет, заказ не будет создан. Это обеспечивает data consistency.

Частые ошибки

1. Большие PR

❌ Проблема: PR с 50+ файлами и 2000+ строк

✅ Решение: Разбивайте на маленькие PR:

  • 1 PR = 1 feature/fix
  • Максимум 400-500 строк кода
  • Легче review, меньше багов

2. Отсутствие тестов

❌ Проблема: Новый код без тестов

✅ Решение:

  • Минимум unit тесты для новой логики
  • Integration тесты для API endpoints
  • Edge cases покрыты

3. Неинформативные комментарии

❌ Проблема: Commit message "fixed stuff"

✅ Решение: Следуйте conventional commits:

feat(auth): add password reset functionality
fix(orders): correct total price calculation
docs(readme): update installation instructions

4. Игнорирование CI

❌ Проблема: PR с failing tests

✅ Решение:

  • Всегда проверяйте локально перед push
  • Исправляйте failing CI немедленно
  • Не мержьте с failing CI

5. Неответ на комментарии

❌ Проблема: Reviewer оставил комментарии, но нет ответа

✅ Решение:

  • Отвечайте на каждый комментарий
  • Если исправили - пометьте resolved
  • Если не согласны - объясните почему

Timeline

Для автора:

  • Создание PR: в течение 1 дня после завершения
  • Ответ на комментарии: в течение 1 дня
  • Исправления: в течение 2 дней

Для reviewer:

  • Первый review: в течение 1 рабочего дня
  • Повторный review: в течение 1 дня после исправлений

Merge:

  • После 1+ approval и прохождения CI
  • Автор мержит свой PR (или reviewer, если автор не доступен)

Полезные команды

# Обновить PR после комментариев
git add .
git commit -m "fix: address review comments"
git push origin feature/your-branch

# Rebase на main перед merge
git fetch origin
git rebase origin/main
git push --force-with-lease

# Squash commits (если нужно)
git rebase -i HEAD~3

# Посмотреть diff с main
git diff main...your-branch

# Найти конфликты
git diff --name-only --diff-filter=U

Ресурсы

Помните: Code review - это не критика, а помощь в написании лучшего кода. Будьте уважительны и конструктивны!