Comments 15
хорошо бы обрабатывать ответы с кодом отличным от 200. Добавляем проверку, строчку будет лучше поместить в функцию.
Во-первых, зачем ради одной самоочевидной строки заводить целую функцию? Во-вторых, в requests и других подобных библиотеках уже есть встроенная функция raise_for_status
, которая выбрасывает исключение при статусах 4xx и 5xx
return False
Пхпшники покусали? Во-первых, выбрасывать исключения при ошибках в питоне абсолютно нормально, та же raise_for_status
так делает. Во-вторых, если есть аллергия на исключения, то можно хотя бы None вернуть, это всё ещё менее странно чем False
Логгирование, контроль исполнения и отладка
Всю эту информацию можно достать напрямую из объекта response или из возникшего исключения, функция response_res
в показанном здесь виде не имеет никакого практического смысла. Кроме того, response_res
ломает статическую типизацию, потому что заменяет объекты с аннотациями типов на нетипизированный словарь — для производства и будущей поддержки кода это ОЧЕНЬ плохо
автоматический сбор данных не очень приветствуется, даже в таком вроде бы легальном поле как получение рсс-ленты.
Бред, RSS по сути своей предназначен именно для автоматизированного сбора данных
словит капчу на второй-третий раз
Капча на RSS — опять бред
fake_useragent.UserAgent().random
И здесь сразу два подозрительных момента. Во-первых, клиент, меняющий свой юзер-агент при каждом запросе — это в принципе странно, а во-вторых, браузеры не умеют читать RSS (за редким исключением в виде браузерных расширений и прочей мелочи) и значит браузерного юзер-агента здесь быть в принципе не должно
не хорошо слать запросы с пустыми cookies, referer и так далее
Ничего из перечисленного быть в принципе не должно, это же RSS
except:
… и получаем невозможность прервать работу программы из-за перехваченных KeyboardInterrupt и SystemExit. К счастью, в следующем примере кода это уже исправлено, хоть и без пояснений
а вот с исключением возникает, вопрос, как его правильно обработать.
Точно не подменой на фейковый Response. Опять аллергия на исключения?
Для одновременных запросов многопоточность вообще хороша
Для одновременных запросов многопоточность вообще не подходит, потому что наплодит кучу жрущих ресурсы потоков, которые при этом будут простаивать без дела в ожидании ответа от сервера. Для io-bound задач специально изобрели асинхронность, которая замечательно работает в одном потоке
logging.info(f"многопоточность сломалась {ex}")
info для критических ошибок, скрытие информации о стеке, скрытие информации о типе ошибки, ужас… Есть же специальный logging.exception
для такого
Если запустить программу в режиме планировщика (например каждые 30 секунд)
Нормальный планировщик должен дожидаться, когда предыдущий запуск завершится
Как выглядит итоговый код
feedparser.parse
зачем-то вызывается два раза, пустая трата процессора. Причём во второй раз не перехватываются возможные исключения (а вдруг внутри feedparser баги есть)
Ещё feedparser сам по себе плох тем, что возвращает результат парсинга в виде нетипизированного словаря, но так как я не в курсе, есть ли достойные альтернативы, то ладно, пусть пока будет
Итого: меняем многопоточность на асинхронность, избавляемся от аллергии на исключения, возвращаем нормальные объекты вместо нетипизированного словаря, выкидываем бесполезный юзер-агент — и получаем гораздо более адекватный и при этом более компактный код:
import asyncio
import logging
import aiohttp
import feedparser
async def harvest_all(url: str) -> None:
try:
timeout = aiohttp.ClientTimeout(total=10)
async with aiohttp.ClientSession(timeout=timeout) as session:
async with session.get(url) as response:
response.raise_for_status()
raw_feed = await response.text()
except Exception:
logging.exception("Failed to get feed %s", url)
return
try:
feed = feedparser.parse(raw_feed)
process_feed(url, feed)
except Exception:
logging.exception("Failed to process feed %s", url)
def process_feed(url: str, feed: feedparser.FeedParserDict) -> None:
if not feed["entries"]:
logging.info("Feed %s has no entries", url)
return
for entry in feed["entries"]:
logging.info("%s", entry["title"])
async def main() -> None:
logging.basicConfig(level=logging.INFO)
rss_list = [
"https://lenta.ru/rss",
"https://habr.com/ru/rss/all/",
]
while True:
tasks = {asyncio.create_task(harvest_all(url)) for url in rss_list}
await asyncio.wait(tasks)
await asyncio.sleep(60)
if __name__ == "__main__":
asyncio.run(main())
Кажется, в самом первом примере проверки 200-го ответа вы перепутали равно с неравно :)
хорошо бы обрабатывать ответы с кодом отличным от 200. Добавляем проверку, строчку будет лучше поместить в функцию.
Во-первых, зачем ради одной самоочевидной строки заводить целую функцию? Во-вторых, в requests и других подобных библиотеках уже есть встроенная функция raise_for_status
, которая выбрасывает исключение при статусах 4xx и 5xx
return False
Пхпшники покусали? Во-первых, выбрасывать исключения при ошибках в питоне абсолютно нормально, та же raise_for_status
так делает. Во-вторых, если есть аллергия на исключения, то можно хотя бы None вернуть, это всё ещё менее странно чем False
Логгирование, контроль исполнения и отладка
Всю эту информацию можно достать напрямую из объекта response или из возникшего исключения, функция response_res
в показанном здесь виде не имеет никакого практического смысла. Кроме того, response_res
ломает статическую типизацию, потому что заменяет объекты с аннотациями типов на нетипизированный словарь — для производства и будущей поддержки кода это ОЧЕНЬ плохо
автоматический сбор данных не очень приветствуется, даже в таком вроде бы легальном поле как получение рсс-ленты.
Бред, RSS по сути своей предназначен именно для автоматизированного сбора данных
словит капчу на второй-третий раз
Капча на RSS — опять бред
fake_useragent.UserAgent().random
И здесь сразу два подозрительных момента. Во-первых, клиент, меняющий свой юзер-агент при каждом запросе — это в принципе странно, а во-вторых, браузеры не умеют читать RSS (за редким исключением в виде браузерных расширений и прочей мелочи) и значит браузерного юзер-агента здесь быть в принципе не должно
не хорошо слать запросы с пустыми cookies, referer и так далее
Ничего из перечисленного быть в принципе не должно, это же RSS
except:
… и получаем невозможность прервать работу программы из-за перехваченных KeyboardInterrupt и SystemExit. К счастью, в следующем примере кода это уже исправлено, хоть и без пояснений
а вот с исключением возникает, вопрос, как его правильно обработать.
Точно не подменой на фейковый Response. Опять аллергия на исключения?
Для одновременных запросов многопоточность вообще хороша
Для одновременных запросов многопоточность вообще не подходит, потому что наплодит кучу жрущих ресурсы потоков, которые при этом будут простаивать без дела в ожидании ответа от сервера. Для io-bound задач специально изобрели асинхронность, которая замечательно работает в одном потоке
logging.info(f"многопоточность сломалась {ex}")
info для критических ошибок, скрытие информации о стеке, скрытие информации о типе ошибки, ужас… Есть же специальный logging.exception
для такого
Если запустить программу в режиме планировщика (например каждые 30 секунд)
Нормальный планировщик должен дожидаться, когда предыдущий запуск завершится
Как выглядит итоговый код
feedparser.parse
зачем-то вызывается два раза, пустая трата процессора. Причём во второй раз не перехватываются возможные исключения (а вдруг внутри feedparser баги есть)
Ещё feedparser сам по себе плох тем, что возвращает результат парсинга в виде нетипизированного словаря, но так как я не в курсе, есть ли достойные альтернативы, то ладно, пусть пока будет
Итого: меняем многопоточность на асинхронность, избавляемся от аллергии на исключения, возвращаем нормальные объекты вместо нетипизированного словаря, выкидываем бесполезный юзер-агент — и получаем гораздо более адекватный и при этом более компактный код:
import asyncio
import logging
import aiohttp
import feedparser
async def harvest_all(url: str) -> None:
try:
timeout = aiohttp.ClientTimeout(total=10)
async with aiohttp.ClientSession(timeout=timeout) as session:
async with session.get(url) as response:
response.raise_for_status()
raw_feed = await response.text()
except Exception:
logging.exception("Failed to get feed %s", url)
return
try:
feed = feedparser.parse(raw_feed)
process_feed(url, feed)
except Exception:
logging.exception("Failed to process feed %s", url)
def process_feed(url: str, feed: feedparser.FeedParserDict) -> None:
if not feed["entries"]:
logging.info("Feed %s has no entries", url)
return
for entry in feed["entries"]:
logging.info("%s", entry["title"])
async def main() -> None:
logging.basicConfig(level=logging.INFO)
rss_list = [
"https://lenta.ru/rss",
"https://habr.com/ru/rss/all/",
]
while True:
tasks = {asyncio.create_task(harvest_all(url)) for url in rss_list}
await asyncio.wait(tasks)
await asyncio.sleep(60)
if __name__ == "__main__":
asyncio.run(main())
Спасибо за квалифицированный комментарий, очень полезные замечания и доработанный код!
Все-таки у оригинального кода есть одно преимущество: его легче понять и доработать. Обратите внимание, как много вашего кода было переиспользовано в предложенном варианте (одно название функции, если я не ошибаюсь?). Часто попытки разобраться как исправить/доработать такой вот продакшн-код занимают больше времени и сил, чем используя типовые паттерны добавить простую обработку ошибок к ничем незамутненному happy path.
Критика конструктивная, особенно по части нетипизированных словарей) и ряд подходов тоже интересные, определенно стоить рассмотреть внедрение. В тоже время предложенный в статье вариант (с учётом отмеченных недостатков) вполне рабочий. По части решения многопоточности, изучу подробнее, тут аргументы не самые убедительные приведены, но с предметом подробно не знаком. В любом случае, пользы от материала получилось много больше чем ожидалось)
Ну знаете, оригинальный вариант тоже будет вполне рабочим, если обернуть его в systemd-сервис с таймером. Оно и упавший процесс перезапустит, и зависший по таймауту прибьет, и несколько экземпляров с разными url можно организовать.
Если хотите разобраться с многопоточностью — попробуйте заставить вашу программу сколько-нибудь прилично реагировать на Ctrl-C, узнаете много нового.
Поюс неплохо было бы If-Modified-Since обрабатывать.
"Чтобы код использовался в рабочем проекте..." - наверно имеет смысл заранее составить чёткое ТЗ что от этого кода ожидается и какие ситуации нужно обрабатывать. Тогда программисту не придётся по ходу дела что-то выдумывать.
В начале материала задача ставится, и дальше идёт объяснение подхода к её решению
нам нужно получать несколько рсс-лент в плотном режиме (например каждую минуту)
Да. Объяснение похода к решению задачи интересное. Но почему этот код может не попасть в прод, если он решает задачу "нам нужно получать несколько рсс-лент в плотном режиме (например каждую минуту) " ? Потому что не совсем правильный подход не соответствует чьей-то жизненной позиции или опыту? Вед в ТЗ эти ньюансы не описаны. Код делает то что нужно.
Ещё можно так писать
try:
response = requests.get()
if response.ok:
...
except requests.RequestException as err:
...
Перечень фидов в коде, а не в отдельном файле, отсутствие самоминимального cli - такой хоккей нам не нужен...
а какой cli может быть для приведенного случая? (фиды в коде, а не в отдельном файле, для наглядности)
Гм. Самый-самый минимум - путь к тому файлу с фидами ). Чуть больше -h, элементарный help. У меня в шаблоне еще настройки логгирования - уровень болтливости, вывод в файл для отладки. Специфически для задачи - размер пула для параллельной обработки (С соответствующей логикой жевания чанками), а то окажется в файле с фидами миллион записей и как-то не хорошо получится. Если предполагаем, что фидов может оказаться таки миллион - еще и всякие таймауты\временное исключение сбойных урлов из списка появится. Для сетевки еще и поддержка прокси, по тому, что фиг знает кто\где\как это все запускать соберется. В случае, если код пойдет в прод, т.е. будет контейнеризован - получение этого из переменных окружения (в click "из коробки", но.).
Всё хорошо, но нужно переписать или почему ваш код не попадает в прод