Reviews - mnogom/python-project-lvl3 GitHub Wiki

go to 54f4725

George Rymarenko 21 июля, 15:39

Константин, добрый день! В целом все хорошо, и мы вышли на финишную прямую.

Предлагаю посвятить еще один круг проверки для исправления мелких недочётов.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L140
Функция _generate_name генерирует наименование, а присваивается результат работы в переменную asset_rel_dir. Логичнее назвать переменную assets_dir_name, тем более, что выше есть переменная page_name.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L141
Соответственно, я бы предложил именование чуть проще, assets_path, к примеру.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L27
Не совсем понятно предназначение этой строчки. Предлагаю разобрать устно.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L28
Эта строка не имеет смысла, если генерируется наименование для директории.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L71
Я бы рекомендовал поместить сразу в вызов urljoin (https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L89). Выносить настолько простое выражение кажется избыточным.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L73
Опечатка - правильнее asset_types.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L82
Как насчет просто assets? Тогда и дальше перебор будет лучше выглядеть: for asset in assets.

[x] https://github.com/mnogom/python-project-lvl3/blob/54f4725f65d3c90a191971b30b8c8b4e06fa8e18/page_loader/loader.py#L94
Можно также asset_path, чтобы все оказалось в одной парадигме именований.

mm: assets_dir_name

go to 3d4375c

George Rymarenko 02 июля, 19:30

Константин, приветствую! В продолжение устного обсуждения фиксирую “на бумаге”, чтобы не ничего не забыли.

Итак, мы совсем близки к завершению работы, осталось немного. В рамках этой итерации код-ревью мы остановились на нескольких ключевых моментах.

[x] https://github.com/mnogom/python-project-lvl3/blob/3d4375ca364c69b940d5b0bc7dbd13813f9b4acb/page_loader/loader.py#L137
Этот шаг нам не потребуется, как только мы вспомним, как работает urljoin. В рамках устного обсуждения мы провели эксперимент с полным, абсолютным и относительным путями, а также дополнительным /. Эту работу необходимо выполнять при замене URL в тэгах.

[x] https://github.com/mnogom/python-project-lvl3/blob/3d4375ca364c69b940d5b0bc7dbd13813f9b4acb/page_loader/loader.py#L15
Тут мы решили, что удобнее будет сначала дробить URL на netloc и path, а после - определять расширение.

[x] https://github.com/mnogom/python-project-lvl3/blob/3d4375ca364c69b940d5b0bc7dbd13813f9b4acb/page_loader/loader.py#L90
urljoin можно вызывать до передачи в _is_local_asset, тогда удастся избежать дублирования кода.

[x] https://github.com/mnogom/python-project-lvl3/blob/3d4375ca364c69b940d5b0bc7dbd13813f9b4acb/page_loader/exceptions.py#L6
Наследоваться необходимо от базового Exception - уже поправил, отлично.

[x] https://github.com/mnogom/python-project-lvl3/blob/3d4375ca364c69b940d5b0bc7dbd13813f9b4acb/tests/test_loader.py#L53
Можно использовать удобную фикстуру https://requests-mock.readthedocs.io/en/latest/pytest.html

go to 327f54f

George Rymarenko 22 июня, 19:14

Константин, приветствую! Проделана большая работа по рефакторингу. Особенно хотелось бы выделить, что теперь все в целом, и основной поток выполнения в частности, выглядят хорошо.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/cli.py#L17
Было бы здорово, если бы не только консольная утилита, но и библиотека умела работать с путем по умолчанию.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L155
Здесь необходимо подумать:

  1. какая проблема решается данным выражением (подсказка: работа с тремя видами URL);
  2. с помощью какого инструмента решается данная проблема. После этого можно принять решение, в каком месте кода оно > должно располагаться.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L51
Если правильно решить задачу, описанную выше, можно значительно упростить данную функцию, сведя все к простой проверке совпадения netloc.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L100
Нужна ли эта проверка?

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L79
Точно ли уместно формировать путь для сохранения assets в данной функции?

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L132
Стоит ли этой проверке находиться выше и определять, вызывать ли данную функцию?

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/tests/test_exceptions.py#L1
Много копипасты, можно через параметризацию значительно сократить. Следующие ресурсы пригодятся: https://requests-mock.readthedocs.io/en/latest/pytest.html#pytest https://requests-mock.readthedocs.io/en/latest/response.html

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/tests/test_exceptions.py#L11
Помним предыдущий комментарий про перенос.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L164
Можно использовать в качестве наименования assets_to_download, тогда в совокупности с проверкой if будет хорошо читаться и ложиться в основой поток.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/tests/configurator.py#L6
Обычно для такого рода задач используются фикстуры (https://docs.pytest.org/en/6.2.x/fixture.html).

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/tests/test_loader.py#L10
Следует использовать параметризацию, тогда в случае провала все варианты все равно пройдут через проверку, а в цикле проверка остановится.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/file_manager.py#L1
Модуль также создает и папки, может стоит рассмотреть file_system.py в качестве наименования?

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/request_manager.py#L1
Как насчет network.py?

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/request_manager.py#L11
Что было первым: сделать запрос или получить ответ? Я бы предложил make_request в качестве наименования.

[x] https://github.com/mnogom/python-project-lvl3/blob/327f54f93f6e0d907d6604da4c2a78fe7135e808/page_loader/loader.py#L140
Задание со звездочкой) Если посмотреть сюда: https://docs.python-requests.org/en/master/user/advanced/#body-content-workflow то можно сделать прогресс бар как в демо.

go to b28b98a

George Rymarenko 25 мая, 20:07

Константин, приветствую! Продолжаем работу над проектом. В рамках этой проверки прежде всего поработаем над основным потоком выполнения.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/loader.py#L76
Ранее была вот эта функция: https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/loader.py#L95
Мы говорили о том, что она делает две вещи: загрузка и сохранение. Сейчас происходит то же самое, но для одного ресурса, а перебор ресурсов переехал в основную функцию download.

Необходимо поработать над основным потоком выполнения. Он может выглядеть вот так:

  1. сгенерировать необходимые имена и пути (для страницы, для ресурсов);
  2. загрузить страницу;
  3. обработать содержимое, подменив URL в тегах и вернув URL ресурсов;
  4. сохранить страницу;
  5. если ресурсы для загрузки, создать папку для ресурсов и загрузить ресурсы (мы ведь получили их URL в пункте 3). Необходимо обратить внимание на пункты 3 и 5 - напрашивается выделение в функции.

И несколько комментариев в целом по проекту.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/example.log#L1
Логи (даже в качестве примера) хранить в репозитории не следует.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/setup.cfg#L18
В проекте используется flake8, а исключение из другого линтера. Когда оно одно, это не проблема, однако в случае множества исключений неясно, какие их них действительно работают, а какие уже нет вследствие, к примеру, рефакторинга.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/Makefile#L8
Флаг force-reinstall, выходит, может быть и не нужен, если помнить про версионирование =)

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/tests/components_tests.py#L1
Модули с тестами принято называть так: test_loader.py, test_exceptions.py и т.д. Другими словами, наименование начинается с test_.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/scripts/page_loader.py#L9
Перенос в импорте лучше оформлять с помощью круглых скоб - скоба открывается, наименование импортируемого объекта с новой строки, в конце на новой строке скоба закрывается.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/errors.py#L1
Это не ошибки, это исключения (exceptions). Разница описана в первом параграфе 8.2 по ссылке: https://docs.python.org/3/tutorial/errors.html#exceptions

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/errors.py#L12
Вот тут исключение переопределяется с наименованием, противоположным по смыслу. Это вводит в заблуждение.

[x] https://github.com/mnogom/python-project-lvl3/blob/b28b98a069b5844d9ac2f7dc0b4ff9e10a0bca49/page_loader/loader.py#L111
Опечатка: assert вместо asset.

go to 239573b

George Rymarenko 04 мая, 12:52

Константин, приветствую! Приношу извинения за длительный ответ.

Итак, теперь у нас есть работающий проект - это здорово! Начнем работу по рефакторингу.

[x] Прежде всего хотел бы отметить хорошо оформленный README - благодаря ему у меня сложилось понимание того, как работает утилита. Есть лишь пара комментариев:

  1. Заголовок Cool badges для бейджей не нужен, мы и так видим, что присутствует в этом разделе, > достаточно сделать вот так: https://github.com/hexlet-boilerplates/python-package;
  2. В справке по утилите, а также ниже в заголовке с примером работы написано debug mod, > а в функционале - debug mode.

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/pyproject.toml#L3
Следует помнить про семантическое версионирование. Проблема в том, что если пакет собрать, а потом я, к примеру, попробую его установить, то у меня это не получится, так как версия будет совпадать с предыдущей. А каждый шаг проекта - новый функционал, следовательно версионность изменялась бы.

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/loader.py#L131
Что произойдет, если в странице не окажется ресурсов, которые потребуется загрузить? Папка все равно будет создана, но останется пустой?

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/loader.py#L64
Эта функция и заменяет теги и загружает их, но функции следует делать что-то одно.

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/request_manager.py#L8
Эту функцию необходимо рефакторить. В настоящий момент идет обработка различных статус кодов, более того, для 40x и 50x идет подмена исключения на ConnectionError, что неверно, так как фактически проблем с соединением нет. За подсказкой можно обратиться к документации: https://docs.python-requests.org/en/master/user/quickstart/#errors-and-exceptions

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/loader.py#L15
Эта функция не извлекает или получает имя, а генерирует его, по этой причине я бы предложил использовать другой глагол, отличный от get. Кроме того, необходимо решить, а не является ли суффикс local избыточным?

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/loader.py#L19
Функция принимает extension - расширение, но в случае с наименованием для папки это не так. Кроме того, возможно ли инкапсулировать работу с расширением в саму функцию, а не получать расширение за ее пределами и передавать, как аргумент?

[x] https://github.com/mnogom/python-project-lvl3/blob/239573bd53850927bfa47bcb5738e63764e1febd/page_loader/loader.py#L32
А почему именно такой набор? Может существует обозначение класса для регулярных выражений?