Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code review #23

Open
BigDeepBlue opened this issue Aug 6, 2022 · 2 comments
Open

Code review #23

BigDeepBlue opened this issue Aug 6, 2022 · 2 comments

Comments

@BigDeepBlue
Copy link
Collaborator

Отличная работа. Замечания незначительные:

  1. nit: Пользуйтесь https://docs.python.org/3/library/http.html#http.HTTPStatus. Это не только повышает читаемость кода, но и является просто хорошей практикой.
  2. Можно было и не переопределять этот метод, в родительском классе все это уже реализовано, а тут вы ничего нового не добавляете к инициализации.
  3. Лишний импорт.
  4. Одним из заданий было: Создайте интеграцию Auth-сервиса и AsyncAPI-сервиса, используя контракт, который вы сделали в прошлом задании. Т.е. добавить middleware, например, которая будет разбирать токен или ходить с ним в сервис авторизации и в зависимости от прав давать тот или иной контент. Например, авторизованным - все, гостям - только списком фильмы без подробностей. Укажите ссылку на сервис выдачи контента, где у вас это реализовано.
@hodosh
Copy link
Owner

hodosh commented Aug 6, 2022

@BigDeepBlue
1-2-3 - done. PR
4) Реализация описана в readme.
Дополню комментарием.
Ссылка на PR изменений по интеграции на стороне Апи контента - https://github.com/hodosh/Async_API_sprint_2_collab/pull/40/files
Идея с middleware имела место быть, однако не хотелось навешивать обработку запросов на все эндпоинты, а только на контент фильмов (почитал в теории, что чтобы middleware навесить только на один route, нужно этот route реализовать как sub app - tiangolo/fastapi#1174). Поэтому было принято решение сделать через обычный Dependency с явной филтрацией результата. Пример - https://github.com/hodosh/Async_API_sprint_2_collab/blob/main/app/src/api/v1/films.py#L57-L62

@BigDeepBlue
Copy link
Collaborator Author

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants