a1d4r / practicum-notifications

Service for notifications
0 stars 0 forks source link

Code review #13

Open BigDeepBlue opened 1 month ago

BigDeepBlue commented 1 month ago

Отлично! Подошли ответственно к проекту 👍

  1. Не нужные пустые файлы notifications-admin/notifications_admin/notifications/tests.py и notifications-admin/notifications_admin/notifications/views.py лучше убрать.
  2. Вот такие строчки не должны пропускаться линтерами. Там нет типизации, а про нее не нужно забывать.
  3. А вот тут разве обязательно коммитить? Наверняка нам хочется атомарности, чтобы подобные операции были в одной транзакции. Если дело в id, то с этим справится flush. Вот тут немного обсуждают https://stackoverflow.com/questions/1316952/sqlalchemy-flush-and-get-inserted-id
  4. Таки да, Celery синхронная и с этим приходится мириться. Но вот так не очень хорошо блокировать цикл событий. Можно попробовать использовать https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor
  5. Пустой файл notifications-worker/notifications_worker/dependencies.py
  6. Для работы с почтой в dev окружении рекомендую https://github.com/mailhog/MailHog. Очень удобный инструмент выступающий в качестве SMTP сервера и web интерфейсом для просмотра всех отправленных сообщений. Вот еще пример https://akrabat.com/using-mailhog-via-docker-for-testing-email/
a1d4r commented 1 month ago
  1. У Mailhog последний релиз был 4 года назад, он уже не поддерживается. Я использовал Mailpit - тоже SMTP сервер с web-интерфейсом, но получше: https://github.com/axllent/mailpit https://chriswiegman.com/2023/03/mailpit-an-updated-alternative-to-mailhog/