discordBot/CODE_REVIEW.md

Код-ревью проекта discordBot

Дата: 2026-07-07 | Тесты: 227 passed, 38 warnings | Python 3.14.0

---

Сводная таблица

Приоритет	Кол-во	Описание
Critical	1	KeyboardInterrupt dead code (C2 — исправлено)
High	7	IndexError в format_weather, translate_weather подстроки, pressure_to_mmhg(0), дубли cogs, asyncio.run() в тестах, HTTPError обработка, on_command_error
Medium	5	Утечка корутины в тестах, None в выводе погоды, embed лимит 2000, type hints, уязвимость help
Low	8	Стиль (color/colour), пустой init, _start_time fallback, RateLimiter на модульном уровне, Session без close, healthcheck, TZ, ISSUES.md

Всего: 21 замечание (C1 — пропущен по решению разработчика)

---

CRITICAL

C2. KeyboardInterrupt не может быть пойман в bot.run() — ИСПРАВЛЕНО KeyboardInterrupt не может быть пойман в bot.run() (dead code)

Файл: bot.py, строки 89-97

except KeyboardInterrupt:
    logger.info("Получен сигнал KeyboardInterrupt")
    self.stop_event.set()
    if self.scheduler:
        self.scheduler.stop()
    asyncio.run_coroutine_threadsafe(
        self.bot.close(), self.bot.loop
    ).result()
    sys.exit(0)

Проблема: self.bot.run(token) -- это блокирующий вызов asyncio.run(), который сам ловит KeyboardInterrupt и завершает event loop. Блок except KeyboardInterrupt никогда не выполнится -- это dead code. Graceful shutdown не работает.

Последствия:

• При Ctrl+C бот завершается резко
• Scheduler не останавливается (self.scheduler.stop() не вызывается)
• bot.close() не вызывается -- подключения к gateway закрываются принудительно
• Лог-сообщение "Получен сигнал KeyboardInterrupt" никогда не появится

Решение: Заменён bot.run() на async with self.bot: + asyncio.run(main()). Добавлены обработчики signal.SIGINT / signal.SIGTERM через _signal_handler(), которые останавливают scheduler и вызывают bot.close() перед sys.exit(0). Обработчик except KeyboardInterrupt после asyncio.run() ловит случай, когда asyncio.run() сам выбрасывает KeyboardInterrupt (дублирующий fallback).

---

HIGH

H1. format_weather_data_for_console -- IndexError при пустом current_condition — ИСПРАВЛЕНО

Файл: utils/pogoda.py, строка 122

current = data.get("current_condition", [{}])[0]

Если API вернул {"current_condition": []}, то [{}][0] не спасёт -- data.get вернёт [] (так как ключ существует), и [0] бросит IndexError.

Тест подтверждает: test_pg_empty_current_condition в tests/test_commands_pg.py ожидает IndexError:

async def test_pg_empty_current_condition(self):
    """current_condition пустой список — код выбрасывает IndexError (баг в коде)."""
    ...
    with pytest.raises(IndexError):
        await cog.pg.callback(cog, ctx)

Последствия: При пустом ответе wttr.in (редко, но возможно) бот упадёт с unhandled exception в Discord.

Рекомендация:

current_condition_list = data.get("current_condition", [])
if not current_condition_list:
    return None
current = current_condition_list[0]

---

H2. translate_weather -- подстроочное совпадение зависит от порядка dict — ИСПРАВЛЕНО

Файл: utils/pogoda.py, строки 96-130

for key, value in mapping.items():
    if key.lower() in en.lower():
        return value

Проблема: "Heavy rain" (11 символов) найдётся внутри "Moderate or heavy rain at times" (31 символ) ДО того, как проверка доберётся до полного ключа. Результат зависит от порядка вставки в dict.

Тест test_translate_longer_key_priority документирует:

def test_translate_longer_key_priority(self):
    """translate_weather ищет key in text, порядок dict важен.
    "Heavy rain" стоит раньше "Moderate or heavy rain at times" в mapping,
    и "heavy rain" in "moderate or heavy rain at times" = True.
    Поэтому совпадёт первым и вернёт "Сильный дождь"."""
    text = "Moderate or heavy rain at times"
    assert translate_weather(text) == "Сильный дождь"

Риск: Если wttr.in вернёт новую фразу, содержащую подстроку существующего ключа, результат будет непредсказуемым при изменении порядка ключей в dict.

Рекомендация: Сортировать ключи по убыванию длины:

SORTED_MAPPING = sorted(mapping.items(), key=lambda x: -len(x[0]))
for key, value in SORTED_MAPPING:
    if key.lower() in en.lower():
        return value

---

H3. pressure_to_mmhg(0) возвращает "--" вместо 0.0 — ИСПРАВЛЕНО

Файл: utils/pogoda.py, строка 162

def pressure_to_mmhg(mb):
    if mb == "--" or not mb:
        return "--"

Проблема: not mb захватывает 0 (falsy value). Давление 0 hPa физически невозможно, но это антипаттерн -- смешивание "отсутствует" и "равно нулю".

Тест документирует: test_pressure_zero в tests/test_pogoda.py:

def test_pressure_zero(self):
    """Нулевое значение — falsy, возвращается '—' (известный баг)."""
    assert pressure_to_mmhg(0) == "—"

Рекомендация:

def pressure_to_mmhg(mb):
    if mb == "--" or mb is None or mb == "":
        return "--"
    try:
        return round(float(mb) * 0.750062, 1)
    except (ValueError, TypeError):
        return "--"

---

H4. Cog-классы добавляются в on_ready -- риск дублирования — ИСПРАВЛЕНО

Файл: bot.py, строки 44-49

@self.bot.event
async def on_ready() -> None:
    logger.info("Бот вошёл как %s", self.bot.user)
    for cog_class in ALL_COMMANDS:
        cog = cog_class()
        await self.bot.add_cog(cog)

Проблема: on_ready может сработать несколько раз (при reconnect gateway). Каждый раз cogs добавляются заново -- дубликаты команд.

Последствия:

• Команды зарегистрируются несколько раз
• При вызове команды discord.py может выбрать случайный дубликат
• Память будет расти с каждым reconnect

Рекомендация:

@self.bot.event
async def on_ready() -> None:
    if self.bot.cogs:  # Уже загружены
        return
    logger.info("Бот вошёл как %s", self.bot.user)
    for cog_class in ALL_COMMANDS:
        cog = cog_class()
        await self.bot.add_cog(cog)

Или лучше -- загрузить cogs через bot.setup_extension() до запуска.

---

H5. asyncio.run() в 50+ тестах вместо async def — ИСПРАВЛЕНО

Файлы:

• tests/test_fetch_cat.py -- 10 вхождений
• tests/test_fetch_rss.py -- 21 вхождение
• tests/test_fetch_weather.py -- 19 вхождений

Пример:

@patch("utils.cat._session.get")
def test_fetch_cat_success(self, mock_get):
    ...
    result = asyncio.run(fetch_cat())
    assert result == "https://example.com/cat.jpg"

Проблема:

1. Каждый asyncio.run() создаёт новый event loop
2. RateLimiter использует asyncio.Lock, который привязан к loop -- при новом loop Lock создаётся заново
3. Конфликт с pytest-asyncio event loop fixture
4. pytest.ini уже настроен: asyncio_mode = auto -- поддерживает async def тесты

Рекомендация: Заменить на async def:

@patch("utils.cat._session.get")
async def test_fetch_cat_success(self, mock_get):
    ...
    result = await fetch_cat()
    assert result == "https://example.com/cat.jpg"

---

H6. fetch_weather -- HTTP-ошибка не ловится retry-блоком

Файл: utils/pogoda.py, строки 20-38

try:
    response = await asyncio.to_thread(_session.get, api_url, timeout=timeout)
    response.raise_for_status()
    return response.json()
except (SSLError, ConnectionError, Timeout):
    # retry с экспоненциальной задержкой
except requests.RequestException as e:
    logger.error("Ошибка при получении данных: %s", e)
    break

Проблема: requests.HTTPError (от raise_for_status()) наследуется от RequestException, поэтому попадает в except requests.RequestException -- делает break без retry.

При HTTP 502/503/504 (временные ошибки сервера) -- бот не сделает retry и сразу перейдёт к fallback. Это не критично (fallback сработает), но теряет преимущество retry-логики для временных ошибок.

Рекомендация: Добавить HTTPError в retry-блок или проверять status code:

except (SSLError, ConnectionError, Timeout, requests.HTTPError):
    if attempt < max_retries - 1:
        delay = 2 ** attempt
        logger.warning(...)
        await asyncio.sleep(delay)
        continue
    break

---

H7. on_command_error -- потенциальный crash при не-Context ошибках

Файл: bot.py, строки 57-77

@self.bot.event
async def on_command_error(ctx: commands.Context, error: Exception) -> None:
    ...
    try:
        await ctx.send("Не удалось выполнить команду. Попробуйте позже.")
    except (discord.NotFound, discord.Forbidden):
        pass

Проблема: Не обрабатываются:

• discord.Forbidden -- бот не имеет прав на отправку (ловится, ок)
• discord.HTTPException -- ошибка отправки (не ловится)
• BadRequest -- сообщение слишком длинное (не ловится)

Рекомендация: Добавить discord.HTTPException и discord.BadRequest в except.

---

H8. Утечка корутины в тестах Scheduler (RuntimeWarning)

Файл: tests/test_morning_runner.py

RuntimeWarning: coroutine 'Scheduler._scheduler_loop' was never awaited

3 предупреждения при тестировании Scheduler.

Причина: patch("asyncio.create_task") перехватывает создание task, но корутина self._scheduler_loop() передаётся в mock и никогда не awaits.

Затронуемые тесты:

• test_next_run_tomorrow_after_time
• test_double_start_no_duplicate
• test_run_morning_empty_embed_fallback

Рекомендация: Патчить _scheduler_loop вместо create_task:

with patch.object(Scheduler, '_scheduler_loop', new=AsyncMock()):
    scheduler = Scheduler(bot)

---

MEDIUM

M1. format_weather_data_for_console -- вывод None в Discord

Файл: utils/pogoda.py

Если API вернул {"temp_C": None}, ф-строка выдаст "Температура: None°C".

Тест подтверждает: test_pg_default_values:

assert "Температура: None°C" in args
assert "Влажность: None%" in args

Последствия: Пользователь увидит None вместо -- в Discord.

Рекомендация: В format_weather_data_for_console использовать helper:

def _safe_val(val, default="—"):
    return default if val is None else val

---

M2. Morning embed может превысить лимит Discord (2000 символов)

Файл: utils/morning_runner.py

Embed description = погода (~200) + 5 статей (~400) + 5 постов (~400) = ~1000 символов. При длинных заголовках статей (до 63 символов каждый с датой и ссылкой) легко превысить 2000.

Последствия: discord.HTTPException -- embed не отправится, пользователь увидит "Не удалось выполнить утренний дайджест."

Рекомендация:

if len(embed.description) > 1900:
    embed.description = embed.description[:1897] + "..."

---

M3. Отсутствуют type hints в утилитах и командах

Файлы без аннотаций:

• utils/pogoda.py -- fetch_weather(), fetch_open_meteo(), translate_weather(), wmo_to_russian(), pressure_to_mmhg(), format_weather_data_for_console(), format_weather_for_embed()
• utils/news.py -- fetch_rss(), _parse_date(), truncate_title(), format_articles()
• commands/pg.py -- pg(self, ctx)
• commands/news.py -- nw(self, ctx)
• commands/cat.py -- cat(self, ctx)
• commands/morning.py -- morning(self, ctx)
• commands/help.py -- hp(self, ctx)
• commands/stats.py -- stats(self, ctx)
• commands/status.py -- status(self, ctx) (частично есть)

Есть аннотации:

• utils/cat.py -- fetch_cat() -> str | None
• utils/rate_limiter.py -- полные аннотации
• utils/morning_runner.py -- полные аннотации
• utils/logger.py -- полные аннотации

Рекомендация: Добавить type hints ко всем публичным функциям. Приоритет: утилиты (pogoda.py, news.py), затем команды.

---

M4. commands/help.py -- уязвимость к None docstring

Файл: commands/help.py, строка 16

desc = (cmd.__doc__ or "".strip()).split("\n")[0].strip()

Если cmd.__doc__ -- None (команда без docstring), результат: "!{name} -- " (пустое описание).

Рекомендация:

doc = (cmd.__doc__ or "").strip()
desc = doc.split("\n")[0].strip() if doc else "Без описания"

---

M5. test_bot.py -- тесты проверяют строки в файле, а не поведение

Файл: tests/test_bot.py

def test_error_handling_code_exists(self):
    with open(ROOT_DIR / "bot.py", encoding="utf-8") as f:
        content = f.read()
    assert "bot.run(token)" in content
    assert "LoginFailure" in content
    assert "HTTPException" in content
    assert "logger.critical" in content

Проблема: Это не юнит-тесты -- это проверка наличия строк в файле. Если код рефакторить (например, заменить bot.run() на async with bot:), тесты упадут, хотя функциональность сохранится.

Рекомендация: Заменить на тесты поведения с mock:

def test_bot_handles_login_failure(self):
    runner = BotRunner()
    with patch.object(runner.bot, 'run', side_effect=discord.LoginFailure("bad token")):
        with patch('sys.exit') as mock_exit:
            runner.run("fake_token")
            mock_exit.assert_called_once_with(1)

---

LOW

L1. commands/cat.py -- color вместо colour

embed = discord.Embed(title="Котик для тебя!", color=discord.Color.orange())

Остальные файлы используют colour. color работает (discord.py принимает оба), но нарушает консистентность.

---

L2. commands/morning.py -- пустой __init__

def __init__(self):
    pass

Лишний код. Можно удалить.

---

L3. commands/status.py -- _start_time fallback

start_time = getattr(ctx.bot, "_start_time", time.time())

Если _start_time не установлен, uptime начнётся с момента вызова команды, а не с запуска.

---

L4. utils/rate_limiter.py -- RateLimiter на модульном уровне

cat_limiter: RateLimiter = RateLimiter(_CAT_RATE, _CAT_BURST)

Создаётся при импорте. asyncio.Lock привязан к loop. Хрупкая зависимость при тестировании.

---

L5. utils/pogoda.py / utils/news.py -- Session без close

_session = requests.Session()

Никогда не закрывается. Для демона не критично, но при graceful shutdown соединения не освобождаются.

---

L6. Dockerfile -- healthcheck через ps aux | grep

HEALTHCHECK CMD ps aux | grep -v grep | grep -q "python bot.py" || exit 1

Проверяет только процесс, а не подключение к Discord.

---

L7. docker-compose.yml -- TZ=UTC5

Для Магнитогорска: Asia/Yekaterinburg (UTC+5) более корректна.

---

L8. 35 DeprecationWarning: asyncio.iscoroutinefunction

DeprecationWarning: 'asyncio.iscoroutinefunction' is deprecated and slated for removal in Python 3.16; use inspect.iscoroutinefunction() instead

Источник: discord.py (внутренний код). Решение -- обновление discord.py или monkey-patch.

---

Статистика тестов

Файл	Кол-во тестов	Статус
test_pogoda.py	93	OK
test_fetch_cat.py	10	OK (asyncio.run)
test_fetch_rss.py	20	OK (asyncio.run)
test_fetch_weather.py	20	OK (asyncio.run)
test_format_articles.py	24	OK
test_commands_pg.py	13	OK
test_bot.py	2	OK (string matching)
test_morning_runner.py	8	OK (RuntimeWarning x3)
test_help_discord.py	2	OK
test_logger.py	9	OK
test_rate_limiter.py	5	OK
test_commands_status.py	6	OK
test_commands_stats.py	5	OK
Итого	227	227 passed, 38 warnings

---

План исправлений (по приоритету)

Этап 1: Critical (безопасность + стабильность)

1. C2: Заменить bot.run() на async with bot: + graceful shutdown — выполнено
2. C1: Ротация токена после ревью — пропущено по решению разработчика

Этап 2: High (баги + надёжность)

3. H1: Защитить format_weather_data_for_console от пустого списка — выполнено
4. H2: Отсортировать ключи translate_weather по убыванию длины — выполнено
5. H3: Исправить pressure_to_mmhg -- not mb -> mb is None or mb == "" — выполнено
6. H4: Добавить защиту от дублирования cogs в on_ready — выполнено
7. H5: Заменить asyncio.run() на async def в fetch-тестах (50+ тестов) — выполнено
8. H6: Добавить HTTPError в retry-блок fetch_weather
9. H7: Добавить HTTPException/BadRequest в on_command_error
10. H8: Исправить утечку корутины в тестах Scheduler

Этап 3: Medium (качество кода)

11. M1: _safe_val() в format_weather_data_for_console
12. M2: Проверка длины embed в morning_runner
13. M3: Добавить type hints в pogoda.py, news.py, команды
14. M4: Fallback "Без описания" в help.py
15. M5: Заменить string-matching тесты в test_bot.py на mock-тесты

Этап 4: Low (стиль + мелочи)

16. L1: color -> colour в cat.py
17. L2: Удалить пустой __init__ в morning.py
18. L3: Документировать _start_time fallback
19. L4: Фабрика RateLimiter для тестов
20. L5: session.close() при shutdown
21. L6: Улучшить healthcheck
22. L7: TZ=Asia/Yekaterinburg