Scraping webelements if found

Question

Im currently working doing a webscrape which in my case if sydsvenskan but will be a template over time for other sites.

I have currently done:

from typing import Optional

import attr
import requests
from bs4 import BeautifulSoup
from loguru import logger
from requests import RequestException
from requests.exceptions import (
    ConnectionError,
    ConnectTimeout,
    ProxyError,
    ReadTimeout,
    ChunkedEncodingError,
    SSLError,
    Timeout
)


@attr.dataclass
class Data:
    store: str = attr.ib(factory=str)
    name: Optional[str] = attr.ib(factory=str)
    info: Optional[str] = attr.ib(factory=str)
    image: Optional[str] = attr.ib(factory=str)


class Info:
    def from_page(url: str) -> Data:
        while True:
            try:
                with requests.get(url) as response:
                    if response.status_code == 404:
                        return Data(store="Sydsvenskan")
                    if response.ok:
                        doc = BeautifulSoup(response.text, 'html.parser')
                    else:
                        response.raise_for_status()

                name = doc.select_one('span.prose-title')
                info = doc.select_one('div.article__preamble-wrapper')
                image = doc.select_one('img.article-image')

                return Data(
                    store="Sydsvenskan",
                    name=name.text.strip() if name else '',
                    info=info.text.strip() if info else '',
                    image=image['src'] if image else '',
                )
            except (
                    ReadTimeout,
                    Timeout,
                    ConnectTimeout,
                    ConnectionError,
                    ChunkedEncodingError,
                    SSLError,
                    ProxyError
            ) as err:
                logger.info(f"{type(err).__name__} at line {err.__traceback__.tb_lineno} of {__file__}: {err}")
                continue

            except RequestException as err:
                logger.exception(f"{type(err).__name__} at line {err.__traceback__.tb_lineno} of {__file__},: {err}")
                raise RequestException from err

            except Exception as err:
                logger.exception(f"{type(err).__name__} at line {err.__traceback__.tb_lineno} of {__file__},: {err}")
                raise Exception from err


def main():
    info = Info.from_page(
        url="https://www.sydsvenskan.se/2021-07-14/flera-anhallna-for-senaste-tidens-skjutningar-i-malmo")
    if info is None:
        logger.info('No new payload')
    else:
        logger.info(f'New payload: {info}')


if __name__ == '__main__':
    main()

The current "template" I have created is meant to be able to add more news sites in the future and to be able to use this as a template. We do know that each site is different so we will never have any similar but the template should make it easy to adept in the future.

I wonder if there is anything I can improve from here? I am mostly "worried" about the return of Data where I use the if else statement inside the dataclass, not sure if that is a correct way to do it?

Answer 1

• Your attrs should not need to have every single property decorated; there's an automatic mode
• On 404 returning a mostly-useless data instance seems like a bad idea.
• Your first tuple of exceptions contains classes that are all derived from RequestException. Is there a need to spell every one of them out? Why not just catch RequestException itself?
• You have a polling loop that continually gets from the server with no delay. That's not a very nice thing to do to a website. Best to add a delay.
• Separating Info and Data is not helpful, and Info on its own contains nothing that deserves to be a class. Just make from_page a class method on Data itself.
• There is no need to check for .ok. raise_for_status will unwind the stack and obviate any other logic.

Data can look like:

@attrs(auto_attribs=True, frozen=True, slots=True)
class Data:
    store: str
    name: Optional[str] = None
    info: Optional[str] = None
    image: Optional[str] = None

    @classmethod
    def from_page(cls, url: str) -> 'Data':
        with requests.get(url) as response:
            response.raise_for_status()
            doc = BeautifulSoup(response.text, 'html.parser')

        name = doc.select_one('span.prose-title')
        info = doc.select_one('div.article__preamble-wrapper')
        image = doc.select_one('img.article-image')

        return cls(
            store="Sydsvenskan",
            name=name and name.text.strip(),
            info=info and info.text.strip(),
            image=image and image['src'],
        )

I suggest that your retry loop be in an outer method and not baked into from_page.

Answer 2

Reinderien's answer is good and covers most of the points I was going to raise, but there are still some things that seem odd to me.

Most contentiously, I'm skeptical that it makes sense for you to be using attr. This is not a library I'm previously familiar with, I had to track down the documentation for it.

• It doesn't look like you're using any of the features attrs offers over dataclasses (argument validation or sanitation; slotted classes).
• Both attrs and dataclasses have a frozen option which I feel pretty strongly should default to True.
• The API for attrs seems to be changing rather a lot from one version to the next. (They actually list this as an advantage, which is ok, but not an argument why you should use them.) In particular, you're using attr.dataclass (which was never part of the official api? I can't find any other documentation of it), and Reinderien is using attrs, which is just an extra wrinkle for a new reader to work through.

Seconding some of Reinderien's points:

• Treat 404 as an error, not a dummy object.
• Don't poll too fast. If the first one fails, trying again too soon can make the problem worse; I suggest a 2-second delay. Also don't poll indefinitely; I would usually have the number of attempts be configurable and default to two.
• Info as a separate class is pointless.

Further points:

• The class name Data isn't great. PageData could be ok. If this is specific to sydsvenskan then call it SydsvenskanData or something.
• This is supposed to be a template system for various sites, right? What's the "template" part (that's the same across sites) and what's the sydsvenskan-specific part? Keep these clearly and cleanly separate from each other, so that the site-specific stuff can be swapped out seamlessly. Is that why you had a distinct Info class? If so, just have a top-level method, but make sure it's clearly associated with the site it's supposed to be used on.
• Reinderien implies that you should move your exception handling up out of your Data-builder function. Clearly you need two layers, with retry-logic somewhere higher and the Data(...) call at the bottom. In keeping with my above point, all of the requests stuff should go in the higher layer with exception-handling and retry-logic; the Data-builder function should take a BeautifulSoup object as its argument.
• Will this mostly be used from the command line, or as a library? In both cases try to expose more of the options to the caller. For example, the argparse library takes some work to understand, but is well worth learning if you want to write CLI tools.