5
\$\begingroup\$

I'm developing scrapy project for scraping news from sites and saving them to Google Sheets. Source repository is here.

I want to ask about my class that must be used as a parent for Scrapy spiders: what can be improved to make it the universal parent of spiders for any news-like web-sites? Also, I want to know how I can improve formatting and docstrings.

Code (v0.7.3):

class TemplateSpider(scrapy.Spider):
    name = None
    """ Spider name. """
    _start_path = None
    """ Relative path to page with news list. Used for `start_urls` field. 
    Minimal value = '/' """
    _start_domain = None
    """ First and only element of `allowed_domains` field. """
    _protocol = None
    """ 'http' or 'https' """

    _xpath_selector_list_tags = None
    _xpath_selector_list_text = None
    _xpath_selector_list_header = None
    """ These three `_xpath_selector_list_*` are used to find needed data by multiple selectors
    in different places. Must contain list of strings or tuple of strings."""
    _xpath_selector_path = None
    """ `_xpath_selector_path` is used to find relative href to article page when scraping from
    news list page. Must contain string."""
    _css_selector_news_list = None
    _css_selector_article = None
    """ These two `_css_selector_*` fields are used to locate news list div tag on news list page and
    to locate article div tag on article page. Must contain string."""

    ### "parse" methods
    def parse(self, response: scrapy.http.Response):
        self._scraped_indexes = self._scraped_in_past
        yield from self._yield_requests_from_response(response)

    def parse_article(self, response: scrapy.http.Response):
        # locate article
        article = self._find_article_in_responce(response)
        # produce item
        yield from self._yield_article_item(
            response,
            text=self._extract_text(article),
            header=self._extract_header(article),
            tags=self._extract_tags(article),
        )

    ### helpers
    def _clear_text_field(self, text: str) -> str:
        string = str(text).replace('\xa0', ' ')
        return string.replace('\n', '')

    def _convert_path_to_index(self, path: str) -> str:
        """ function that extracts unique part from given url."""
        raise NotImplementedError

    def _check_field_implementation(self, field_name: str):
        value = self.__getattribute__(field_name)
        if value is not None:
            return value
        else:
            raise NotImplementedError('Need to define "{}" field.'.format(field_name))

    @property
    def _scraped_in_past(self):
        return fetch_scraped_indexes(self.name)

    ### "yield" methods that returns generators
    def _yield_request(self, path_or_url: str):
        if '://' in path_or_url:
            url = path_or_url
            # extracting relative path from url
            _protocol = self._protocol + '://'
            path = path_or_url[path_or_url[len(_protocol):].find('/') + len(_protocol) + 1:]
        else:
            path = path_or_url
            url = '{protocol}://{host}/{path}'.format(protocol=self._protocol, host=self.allowed_domains[0], path=path)
        index = self._convert_path_to_index(path)
        if index not in self._scraped_indexes:
            yield scrapy.http.Request(url=url,
                                      callback=self.parse_article,
                                      meta={'index': index})

    def _yield_article_item(self, response: scrapy.http.Response, **kwargs):
        yield EventItem(
            url=response.url,
            index=response.meta['index'],
            **kwargs
        )

    def _yield_requests_from_response(self, response: scrapy.http.Response):
        """ Yields requests with `parse_article` callback.
        Takes response, finds, extracts news list, extracts from every path and generates requests."""
        for selector in response.css(self._css_selector_news_list):
            path = selector.xpath(self._xpath_selector_path).extract_first()
            yield from self._yield_request(path)

    ### "find" methods that returns Selectors
    def _find_by_xpath_list(self, article: scrapy.selector.SelectorList, xpath_string_selectors_list: list or tuple) -> scrapy.selector.SelectorList:
        selector_list = article.xpath(xpath_string_selectors_list[0])
        for string_selector in xpath_string_selectors_list[1:]:
            selector_list.extend(article.xpath(string_selector))
        return selector_list

    def _find_article_in_responce(self, response: scrapy.http.Response) -> scrapy.selector.SelectorList:
        return response.css(self._css_selector_article)

    def _find_news_list_in_responce(self, response: scrapy.http.Response) -> scrapy.selector.SelectorList:
        return response.css(self._css_selector_news_list)

    def _find_tags_in_article(self, article: scrapy.selector.SelectorList) -> scrapy.selector.SelectorList:
        return self._find_by_xpath_list(article, self._xpath_selector_list_tags)

    def _find_text_in_article(self, article: scrapy.selector.SelectorList) -> scrapy.selector.SelectorList:
        return self._find_by_xpath_list(article, self._xpath_selector_list_text)

    def _find_header_in_article(self, article: scrapy.selector.SelectorList) -> scrapy.selector.SelectorList:
        return self._find_by_xpath_list(article, self._xpath_selector_list_header)

    ### "extract" methods that returns strings
    def _extract_tags(self, article: scrapy.selector.SelectorList) -> str:
        return convert_list_to_string(self._find_tags_in_article(article).extract(), ',')

    def _extract_text(self, article: scrapy.selector.SelectorList) -> str:
        return convert_list_to_string(self._find_text_in_article(article).extract(), '', handler=self._clear_text_field)

    def _extract_header(self, article: scrapy.selector.SelectorList) -> str:
        return self._find_header_in_article(article).extract_first()

    @property
    def allowed_domains(self):
        return [self._check_field_implementation('_start_domain'), ]

    @property
    def start_urls(self):
        return ['{}://{}/{}'.format(self._check_field_implementation('_protocol'),
                                    self._check_field_implementation('_start_domain'),
                                    self._check_field_implementation('_start_path')), ]

About used but not declared functions:

  • convert_list_to_string works similar to join, but calls handler(item) for every item in the list before adding it to output string
  • fetch_scraped_indexes uses Scrapy Cloud API to fetch all finished jobs from last week and return a list of index field in every item.

About index - it part of the article URL that is unique for that article and can be extracted from any article URL on the web-site to indicate the page and not scrape it twice.

Working usage examples in the files in this folder.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Great job overall! I like how simple and readable the usage sample classes are.

Here are some of my high-level overview thoughts.

Code Organization

The class feels more like a "God class", since it combines a lot of not directly related things - for instance, it has this _clear_text_field helper, different _find* and _extract* methods and parse* callbacks altogether in a single place. One way to approach the problem is to apply the "Mixins" pattern.

Readability

You've provided quite a lot of comments, but some of the methods are not documented and it is not clear what is their purpose. Look at the _find* and _extract* methods - it is not easy to understand and keep track of what is going on there.

Code Quality

  • you are using docstring convention of putting documentation into triple-quoted strings for comments. Instead, use a single # to denote a comment on a single line (PEP-8 reference).

  • also, a comment precedes the line it is attached to, not the other way around.

  • you have a couple typos - you meant to call the methods _find_article_in_response and _find_news_list_in_response instead of _find_article_in_responce and _find_news_list_in_responce.

  • since SelectorList is a subclass of a regular list, you should be able to use the "selector list comprehension" simplifying the _find_by_xpath_list() method:

    def _find_by_xpath_list(self, article: scrapy.selector.SelectorList, xpath_string_selectors_list: list or tuple) -> scrapy.selector.SelectorList:
        return scrapy.selector.SelectorList([article.xpath(string_selector) for string_selector in xpath_string_selectors_list])
    
  • you can import Request directly from scrapy, replacing yield scrapy.http.Request with yield scrapy.Request.

  • the _yield_request() method may use urlparse module tools

\$\endgroup\$
6
  • \$\begingroup\$ Thank you for your feedback! My triple-quote commenting style comes from using the flask-admin library where you can find a lot of them: example of the class \$\endgroup\$ Jun 26, 2017 at 9:12
  • \$\begingroup\$ How should I implement and use Mixins if some methods need documentation that depends on most of other methods? Have you some quick plan how should I organize methods in Mixins? \$\endgroup\$ Jun 27, 2017 at 19:03
  • \$\begingroup\$ @IlliaAnanich at least, you can start with putting _find* and _extract* into a mixin and see how it looks - these methods feel like they should be grouped together. Thanks. \$\endgroup\$
    – alecxe
    Jun 27, 2017 at 19:06
  • \$\begingroup\$ That's my first try to use Mixins here. Previous commit contains all other changes from your answer. \$\endgroup\$ Jun 27, 2017 at 20:28
  • \$\begingroup\$ @IlliaAnanich I think it looks cleaner than before. Please consider moving the mixing into a separate, say, mixins.py module. Thanks. \$\endgroup\$
    – alecxe
    Jun 27, 2017 at 20:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.