search

MultikPatin / EFS_content_delivery

Сервис выдачи контента
1 stars 0 forks source link

Code review #33

Open BigDeepBlue opened 7 months ago

BigDeepBlue commented 7 months ago

Все отлично! Тесты очень подробные, с SOLID тоже все ок 👍

  1. В скриптах ожидания ES и Redis в тестах (tests/functional/utils/wait_for_es.py и tests/functional/utils/wait_for_redis.py) вместо вечного цикла, в который можно попасть при возникновении сложностей, лучше использовать backoff из прошлых спринтов. Он более функционален в сравнении с вечным циклом, теоретически он может сигнализировать команде поддержки после определенного количества попыток.
  2. Пустой файл tests/functional/utils/helpers.py
  3. С развитием проекта в тестах фикстур будет все больше и больше, и их было бы неплохо систематизировать. Можно вынести в отдельный файл (затем добавлять другие файлы, логически разделяя их) и тем самым облегчим conftest.py. Для этого в conftest.py есть pytest_plugins
  4. Не используйте прямое указание кодов ответов сервера (200, 400 и т.д.), применяйте http.HTTPStatus. Дело в том, что в коде проекта найти HTTPStatus.OK значительно проще чем найти 200, кроме того этим мы защитимся от опечаток, да и читаемость кода повысится.
  5. В src/api/endpoints/v1 много раз встречается вот такая конструкция в параметрах методов: 
      page_number: Annotated[
          int,
          page_number_query,
      ] = 1,
      page_size: Annotated[
          int,
          page_size_query,
      ] = 50

    Можно попробовать вынести эту пару в отдельный класс, какой-нибудь PaginatedParams, инжектить его в качестве зависимости в параметры методов и работать с ними как с объектом. Т.о. параметры пагинации будут в одном месте и при необходимости легко будет их изменить сразу и везде.

  6. С SOLID все хорошо. Вдруг будет интересно. У меня есть небольшой набросок, https://github.com/RomanAVolodin/FastAPI_DI/tree/feature/DI, вся работа идет с абстрактными классами, которые выступают в роли интерфейсов, а сопоставление абстрактного класса и его реализации происходит в одном месте, и если будет принято решение использовать другую реализацию, то достуточно будет внести изменение в одном месте кода.
BigDeepBlue commented 7 months ago

LGTM