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

Валентин Хомутенко 17 сент., 11:46

Привет! Стало намного лучше, сейчас в целом видно какую-то понятную последовательность обработки данных в структуре проекта. Теперь сильно проще ориентироваться в коде и находить нужную функциональность. Хотелось бы еще немного почистить код на уровне самих функций. Местами есть необязательные усложнения или просто лишние действия, давай попробуем убрать их и будет совсем хорошо. Я тут оставлю ссылки на конкретные места с конкретными рекомендациями, но в качестве упражнения советую еще и самостоятельно пройтись глазами по каждой функции и подумать, можно ли её как-то упростить.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/page_loader.py#L7 Набор этих тэгов и атрибутов непосредственно относится к логике парсинга html страницы, поэтому логично унести эти списки туда, а не передавать их в качестве параметра.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L22 Кажется два раз requests.get тут не нужен. Можно же один раз сделать get, получить ответ, сделать raies_for_status и продолжить использовать этот ответ дальше.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L24 Не очень понял, зачем нужен этот assert. В каком случае response может быть None?

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L28 Тут поще ловить базовый requests.RequestsException, чем явно перечислять все возможные ошибки — легко что-то забыть.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L53 В try except заворачиваем непосредственно те операции, которые могут упасть. Если хотим поймать FileNotFoundError, то заворачиваем в try except именно кусок по работе с файлами. Так будет проще понять, обрабатываем ли мы все нужные ошибки или нет. + Тут кроме FileNotFoundError могут быть и другие проблемы — место на диске закончилось, прав на запись нет и т.д. Если нет задачи четко различать, то можно просто OSError ловить.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/normalize_data.py#L75 Опять же, тут следует заворачивать только mkdir(). + Ловить базовый OSError где-то.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L46 Вопрос с точки зрения структуры проекта в целом — точно ли нам нужно предварительно сохранять страницу в файл? Получается мы по-сути страницу записываем на диск дважды — один раз в исходном виде, другой раз в модифицированном. Если исходный вид нам на самом деле в файле не нужен, может быть мы тут просто строкой вернем его наружу из функции и будем дальше использовать?

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/normalize_data.py#L52 Не очень понял, зачем все эти манипуляции с position. Проверь пожалуйста по тестам, точно ли это нужно. Если нужно, то я бы порекомендовал оставить комментарий с пояснением, какой кейс мы тут обходим.

Многовато дублирования convert_path_name получилось. По-сути же нам в любом случае нужно convert_path_name сделать для имени файла, все эти if’ы отвечают только за добавление / удаление расширения. Можем мы как-то здесь реорганизовать код, чтобы convert_path_name делался один раз только?

Абстрактный параметр flag с текстовым значением — не очень понятно о чем. Лучше сформулировать параметр как логическое значение исходя из того, что он фактически делает. Например можно было бы сделать флаг keep_extension = True/False, или наоборот strip_extension = True/False, тогда сразу очевидно было бы, что тут происходит.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L60 Кажется тут логика для определения локальности ресурса немного переусложнена. Разве нам не достаточно просто сравнить netloc?

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L79 Если у картинки по ошибке добавят атрибут href — его не нужно отрабатывать. Давай сделаем конкретный маппинг тэга в нужный атрибут, чтобы для img мы искали именно src, для link — href и т.д.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L83 Если if url_tag вернул True, то он точно не ‘’ и не None, что мы тут проверяем?

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L89 — Сейчас ресурсы скачиваются целиком за раз, а не чанками. — Сейчас если возникла ошибка при скачивании ресурса — программа падает. Кажется было бы логично продолжать загружать остальные ресурсы просто. Ничего страшного, если пара картинок у нас не покажутся — зато покажем все остальное.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/web_data_processing.py#L98 Не очень понял, в чем смысл прогресс бара. Он появляется только когда уже все скачалось и пробегает сразу от начала до конца. Нам же нужно визуализировать реальный процесс загрузки.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/page_loader/colors.py#L1 Эти коды не являются кросслпатформенными, в некоторых терминалах/операционных системах может не работать. Лучше взять готовый пакет типа colorama, который умеет правильно подставлять цвета для разных платформ.

По тестам: https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/tests/test_.py#L64 см. прошлое ревью — это не очень корректный тест. Не нужно сравнивать два скачанных раньше файла и надеяться, что сейчас твой loader именно так и работает.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/tests/test_.py#L74 Лучше сделать кастомные типы исключений и проверять конкретно на тип исключения, а не разбирать сообщение об ошибке.

https://github.com/Serggi0/python-project-lvl3/blob/d49c1b1ba5845b801580cde8036d212c4e75df3f/tests/test_.py#L113 Почему не просто фикстура requests_mock?