georotor / movies_assistant

Сервис интеграции между сервисом поиска фильмов и голосовыми помощниками Алиса и Маруся.
0 stars 1 forks source link

Code review #15

Closed BigDeepBlue closed 1 year ago

BigDeepBlue commented 1 year ago

Добрый день.

Отличный у вас диплом получился!!! Видно, что был проделан большой объем работы. У меня совсем немного замечаний.

  1. В логгировании не стоит использовать f-string,.format() , а делать через printf-style примерно так - logging.info("message %s", param) printf-style строка не форматируется до самого последнего момента. То есть если использовать f-string, то все вычисления по формированию строки происходят сразу, а если использовать %s и параметры, то только непосредственно перед выводом данных. А если например level handler будет в конфиге поставлен в critical, то этот лог никуда не отправится, а значит опять же строка форматироваться не будет. https://okomestudio.net/biboroku/2020/04/on-lazy-logging-evaluation/
  2. Было бы неплохо избавиться от magic numbers. Например тут сразу непонятно что за 1 и почему 1
  3. Этот location будет отправлять в сервис запросы вроде /ihackyoursserver. И такими запросами мы будем грузить сервис, а могли бы просто отклонить. Как смотрите на то, чтобы доработать этот момент и сделать более тонко работающий location, который будет резолвить только нужные нам запросы (/api/v1/), используя регулярное выражение. location <здесьнапиширегулярку> { try_files $uri http://assist:8000; }
georotor commented 1 year ago

Приветствую!

  1. Исправлено.
  2. Не придумал как в указанном месте избавиться от 1, там идет просто выбор шаблона для фоматрирования, поэтому изменил названия пременных и форматирование, чтобы было более понятно что там происходит.
  3. Сделал location /api, чтобы сохранить рабочий swagger, если вдруг мы решим что он нам не нужен то можно сделать location ~* ^/api/v1/(alisa|marusia)/$ { } или фиксированные location = /api/v1/alisa и location = /api/v1/marusia.
BigDeepBlue commented 1 year ago

Удачи на защите!