search

dimk00z / Async_API_sprint_1

4 спринт курса и первая часть асинхронного API для кинотеатра
0 stars 0 forks source link

Code review #30

Open BigDeepBlue opened 3 years ago

BigDeepBlue commented 3 years ago

Вы хорошо поработали! Оценил makefile - отличная практика! Также отмечу очень подробную документацию.

Я обратил внимание, что задачи вы действительно разбили между собой, это хорошо. Но есть один нюанс - при командой работе необходимо синхронизировать свои решения, особенно, если работаете над одной частью проекта. У вас это до конца не получилось. В сервисе для персон вы используете cached из aiocache, а в остальных сервисах базовый класс со своей собственной реализацией кэширования. В реальном проекте такое разногласие в коде недопустимо. Поэтому первое с чего я предлагаю вам начать - это синхронизировать ваши решения. Выбрать единый инструмент и сделать три сервиса стилистически одинаковыми. Также проверьте другие места по этому признаку. Теперь к более мелким замечаниям (в порядке обнаружения):

  1. В docker-compose укажите конкретную версию redis
  2. С dockerfile поспешили. Давайте подправим, чтобы у нас зависимости копировались до pip, а остальной код после. Плюс две команды с pip объедините в одну через &&. Так сэкономим 1 слой образа
  3. Тут вижу у вас импорты перемешаны. Запустите isort, который я увидел у вас в setup.cfg. Если запускали и такая же картина, то используйте эти параметры https://pycqa.github.io/isort/docs/configuration/options.html#known-third-party
  4. С версии python 3.9 для всех стандартных коллекций (list, dict etc) не нужен модуль typing - https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections. Поправьте во всем проекте (у вас как-то пополам реализовано, видимо кто-то в курсе, кто-то - нет)
  5. Остальные комменты в мр: https://github.com/dimk00z/Async_API_sprint_1/pull/29

p.s. Комменты в мр правьте в соотвествии со своим решением на счет единообразия сервисов. Если сделаете везде aiocache, то часть комментов можно просто зарезолвить

BigDeepBlue commented 3 years ago

Хорошие доработки! Теперь сервисы максимально эффективны)

Еще несколько рекомендаций:

  1. На продакшене голый uvicorn не рекомендуется запускать, подробнее тут: https://www.uvicorn.org/deployment/#gunicorn
  2. У redis-cache неплохо бы дать возможность задать db для кэша. По умолчанию все либы redis пишут в db=0, но обычно хочется разнести cache и другие данные в редисе, даже разные по приоритету кэши можно разносить. Давайте этот параметр добавим, чтобы db можно было задать через конфиг. А на будущее еще учтите, что редис по-умолчанию может съесть всю оперативку и если вы предполагаете его использовать в качестве именно кэша, то можно использовать спец. конфигурацию ввиде LRU-кэша (https://redis.io/topics/config), в самом низу подробности. Это реализовывать не надо, просто имейте ввиду

На первом ревью увлекся вашей работой и забыл заглянуть в ETL:

  1. Тут было бы хорошо превратить класс в абстрактный
  2. Эту структуру хорошо бы вынести на уровень модуля
  3. Нет никакой необходимости нарушать здесь PEP8 и писать параметр в camel case
BigDeepBlue commented 3 years ago

Принято!

И последний совет перед следующим спринтом: когда будете декомпозировать задачу - сразу прикиньте какие модули будут зависеть друг от друга, соберитесь и обсудите реализацию. Тогда не придется перепиливать) сразу будет спроектирован хороший сервис для тестирования. Удачи!