1. - Serggi0/python-project-lvl3 GitHub Wiki

Валентин Хомутенко 28 авг., 14:29

Привет! Поздравляю с завершением третьего проекта :) В этом ревью обсудим общие вещи по оформлению репозитория, конфигурациям, тестам и пр., и немного обсудим общую архитектуру. На уровень отдельных функций и кода пока спускаться не будем, т.к. что-то еще может поменяться к следующему ревью — тогда и посмотрим более внимательно уже.

В README не хватает текстового описания инструкций запуска. Хорошо, что есть asciinema — это неплохая демонстрация возможностей проекта. Но по ней не очень удобно воспроизводить команды, т.к. их нужно перепечатывать с видео и т.д. Поэтому обычно такие вещи как инструкции по установке, примеров запуска и т.д. в том числе дублируют текстом в виде блоков кода, которые удобно скопировать и вставить сразу в терминал.

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/pyproject.toml#L14 В зависимостях два прогресс бара — tqdm и progress, причем первый используется только при записи файла. https://github.com/Serggi0/python-project-lvl3/blob/cfe367ba3fe62ef6594fb1cbdba68eb417f6b3e4/page_loader/page_loader.py#L71 Точно там прогресс бар вообще нужен?

https://github.com/Serggi0/python-project-lvl3/blob/main/myapp.log Логи не нужно коммитить в репозиторий, это временные файлы.

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/Makefile#L5 Опечатка в url в команде make page-loader.

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/.github/workflows/github_action_.yaml#L36 Токены доступа лучше не хранить в репозитории в открытом виде, иначе у всех есть доступ к твоему аккаунту в сервисе :) Тут это не очень страшно, в худшем случае какой-нибудь негодяй тебе запушит неправильный code coverage. Но лучше себя сразу приучать внимательно к таким вещам относиться. У CI/CD систем обычно есть всегда какой-то механизм секретов для безопасного хранения таких данных https://docs.github.com/en/actions/reference/encrypted-secrets

По тестам: Сейчас недостаток тестов, что они все тестируют отдельные функции из проекта. Нет тестов, которые проверили, что загрузка в целом работает правильно на всех этапах. Это не очень здорово, потому что при первом же рефакторинге, когда мы захотим какие-то функции объединить, какие-то выделить в новые, придется переписывать сразу еще и много тестов. В этом случае тесты не помогают при рефакторинге, а наоборот становятся обузой команды разработки. Чтобы не доводить до такого, обычно в проектах нужно иметь хотя бы минимальное количество более общих тестов. У нас есть статья про это в блоге, можно ее посмотреть: https://ru.hexlet.io/blog/posts/how-to-test-code

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/tests/test_.py#L113 Нам не нужны специальные библиотеки, чтобы просто сравнить два файла на равенство. Нас же не интересует, как именно изменились картинки, мы просто хотим проверить, что содержимое файлов совпадает.

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/tests/test_.py#L124 Не очень понял, что проверяет этот тест. Если web_page_after.html это страница, которую сохранил наш загрузчик, то нужно явно эту страниц во время теста и создавать, иначе мы тут ничего особенно не проверим. Представь, что мы допустили баг в нашем загрузчике и теперь страница загружается неправильно. Тест это не покажет, потому что он все еще работает со старым «правильным файлом».

скрипт: https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/page_loader/scripts/page_loader.py#L24 Не очень понял, зачем тут делать запрос получения страницы перед вызовом самой функции download. Если смысл только в том, чтобы проверить ошибку, то она бы возникла же и так, просто внутри функции download — зачем дублировать?

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/page_loader/page_loader.py#L79 Много где в проекте дублируется этот вызов check_url, который видимо нужен только чтобы проверить, что ошибка не возникнет при выполнении запроса. См. выше — не нужно этого делать, мы же все равно в какой-то момент потом выполним сетевой запрос и если что там ошибку и поймаем.

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/page_loader/scripts/page_loader.py#L27 Я писал небольшую заметку в группу про принципы обработки ошибок, можно там посмотреть, как организовать исключения в приложении. https://hexlet-students.slack.com/archives/G01JYKHGNNN/p1622482868017700

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/page_loader/normalize_data.py#L22 Как к нам может прийти не строка в качестве url?

https://github.com/Serggi0/python-project-lvl3/blob/c6af05e4b49ebdebfc85ee748937f325a2bc6648/page_loader/page_loader.py#L82 Код в целом получился довольно сложным, потому что нет четкого деления на этапы обработки страницы. Сложно сходу понять, где найти тот или иной блок функциональности. Типа «где тут скачивается собственно страница?» Давай попробуем сделать такое упражнение. Выпиши просто списком, какие этапы у нас можно выделить в проекте. «Загрузить содержимое страницы по url» и т.д. При этом можно отмечать рядом, подразумевает ли этот шаг какие-то побочные эффекты или нет. После этого нужно попробовать сделать структуру главной функции такой, чтобы она как можно точнее отражала эту последовательность шагов. Так в коде будет намного проще разбираться и искать ошибки. 
Вот тут есть еще дополнительный материал про построение функций: https://ru.hexlet.io/blog/posts/sovershennyy-kod-proektirovanie-funktsiy