From d62fac7718b400928a48e171afee2bcae39582cd Mon Sep 17 00:00:00 2001 From: deadzilla Date: Tue, 7 Jul 2026 21:57:49 +0500 Subject: [PATCH] =?UTF-8?q?=D0=A3=D0=B4=D0=B0=D0=BB=D1=91=D0=BD=20=D1=84?= =?UTF-8?q?=D0=B0=D0=B9=D0=BB=20CODE=5FREVIEW.md?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CODE_REVIEW.md | 564 ------------------------------------------------- 1 file changed, 564 deletions(-) delete mode 100644 CODE_REVIEW.md diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md deleted file mode 100644 index bf3eb88..0000000 --- a/CODE_REVIEW.md +++ /dev/null @@ -1,564 +0,0 @@ -# Код-ревью проекта 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 - -```python -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 - -```python -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`: - -```python -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. - -**Рекомендация:** - -```python -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 - -```python -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` документирует: - -```python -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. - -**Рекомендация:** Сортировать ключи по убыванию длины: - -```python -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 - -```python -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`: - -```python -def test_pressure_zero(self): - """Нулевое значение — falsy, возвращается '—' (известный баг).""" - assert pressure_to_mmhg(0) == "—" -``` - -**Рекомендация:** - -```python -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 - -```python -@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 - -**Рекомендация:** - -```python -@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 вхождений - -**Пример:** - -```python -@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`: - -```python -@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 - -```python -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: - -```python -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 - -```python -@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`: - -```python -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`: - -```python -assert "Температура: None°C" in args -assert "Влажность: None%" in args -``` - -**Последствия:** Пользователь увидит `None` вместо `--` в Discord. - -**Рекомендация:** В `format_weather_data_for_console` использовать helper: - -```python -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 не отправится, пользователь увидит "Не удалось выполнить утренний дайджест." - -**Рекомендация:** - -```python -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 - -```python -desc = (cmd.__doc__ or "".strip()).split("\n")[0].strip() -``` - -Если `cmd.__doc__` -- `None` (команда без docstring), результат: `"!{name} -- "` (пустое описание). - -**Рекомендация:** - -```python -doc = (cmd.__doc__ or "").strip() -desc = doc.split("\n")[0].strip() if doc else "Без описания" -``` - ---- - -### M5. `test_bot.py` -- тесты проверяют строки в файле, а не поведение - -**Файл:** `tests/test_bot.py` - -```python -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: - -```python -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` - -```python -embed = discord.Embed(title="Котик для тебя!", color=discord.Color.orange()) -``` - -Остальные файлы используют `colour`. `color` работает (discord.py принимает оба), но нарушает консистентность. - ---- - -### L2. `commands/morning.py` -- пустой `__init__` - -```python -def __init__(self): - pass -``` - -Лишний код. Можно удалить. - ---- - -### L3. `commands/status.py` -- `_start_time` fallback - -```python -start_time = getattr(ctx.bot, "_start_time", time.time()) -``` - -Если `_start_time` не установлен, uptime начнётся с момента вызова команды, а не с запуска. - ---- - -### L4. `utils/rate_limiter.py` -- RateLimiter на модульном уровне - -```python -cat_limiter: RateLimiter = RateLimiter(_CAT_RATE, _CAT_BURST) -``` - -Создаётся при импорте. `asyncio.Lock` привязан к loop. Хрупкая зависимость при тестировании. - ---- - -### L5. `utils/pogoda.py` / `utils/news.py` -- Session без close - -```python -_session = requests.Session() -``` - -Никогда не закрывается. Для демона не критично, но при graceful shutdown соединения не освобождаются. - ---- - -### L6. `Dockerfile` -- healthcheck через `ps aux | grep` - -```dockerfile -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. [x] C2: Заменить `bot.run()` на `async with bot:` + graceful shutdown — **выполнено** -2. [ ] C1: Ротация токена после ревью — **пропущено по решению разработчика** - -### Этап 2: High (баги + надёжность) - -3. [x] H1: Защитить `format_weather_data_for_console` от пустого списка — **выполнено** -4. [x] H2: Отсортировать ключи `translate_weather` по убыванию длины — **выполнено** -5. [x] H3: Исправить `pressure_to_mmhg` -- `not mb` -> `mb is None or mb == ""` — **выполнено** -6. [x] H4: Добавить защиту от дублирования cogs в `on_ready` — **выполнено** -7. [x] 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`