6
\$\begingroup\$

I use the following script to download the result of a SEDE query and scrape a specific column from it using BeautifulSoup:

import json


def extract_column(soup, colname):
    """
    Returns a generator of cell values in selected column.

    For simple columns like timestamp, a cell value can be simple,
    for example: 1414433013197

    For more complex columns like Post Link, a cell value can be an object,
    for example:

      {
        "id": 68102,
        "title": "Bash Script - File Comment out & Notate"
      }

    :param soup: a bs4 (BeautifulSoup) object
    :param colname: name of the SEDE column to extract
    :return: generator of cell values in selected column
    """
    def get_column_index():
        for index, info in enumerate(columns):
            if info['name'] == colname:
                return index
        return -1

    for script in soup.findAll('script'):
        result_sets_col = 'resultSets'
        if result_sets_col in script.text:
            start = script.text.rindex('{', 0, script.text.index(result_sets_col))
            end = script.text.index('}', script.text.index('querySetId')) + 1
            data = json.loads(script.text[start:end])

            results = data[result_sets_col][0]
            columns = results['columns']
            rows = results['rows']

            column_index = get_column_index()
            if column_index > -1:
                for row in rows:
                    yield row[column_index]

Unit tests:

import os
import unittest

from bs4 import BeautifulSoup
from robosanta.stackexchange.sede import extract_column

BASE_DIR = os.path.dirname(__file__)

SEDE_OUTPUT_HTML = os.path.join(BASE_DIR, 'sede-output.html')
POST_ID_COLUMN = 'Post Link'
DATE_COLUMN = 'CreationDate'
ROW_COUNT = 49


def new_soup():
    with open(SEDE_OUTPUT_HTML) as fh:
        return BeautifulSoup(fh)


class TestGetColumn(unittest.TestCase):
    def extract_column(self, colname):
        return extract_column(new_soup(), colname)

    def extract_post_link(self):
        return self.extract_column(POST_ID_COLUMN)

    def extract_date(self):
        return self.extract_column(DATE_COLUMN)

    def test_row_count_of_post_link(self):
        self.assertEqual(ROW_COUNT, len(list(self.extract_post_link())))

    def test_first_post_link(self):
        expected = {
            'title': 'TinyMVC Model / Plugin how to implement?',
            'id': 12144,
        }
        self.assertEqual(expected, next(self.extract_post_link()))

    def test_row_count_of_date(self):
        self.assertEqual(ROW_COUNT, len(list(self.extract_date())))

    def test_first_date(self):
        self.assertEqual(1338304381360, next(self.extract_date()))

    def test_nonexistent_column(self):
        with self.assertRaises(StopIteration):
            next(self.extract_column('nonexistent'))


if __name__ == '__main__':
    unittest.main()

You can find the content of sede-output.html (the test data) here.

I'm looking for any and all suggestions, especially concerning:

  • Improving the main implementation (the scraping)
  • Improving the way to test this
\$\endgroup\$
  • \$\begingroup\$ I would suggest taking a look at scrapy. \$\endgroup\$ – Quill Dec 14 '15 at 13:57
  • \$\begingroup\$ @Quill I think you should include your reasoning with suggestions like that. For example, do you see some obvious advantages of scrapy over beautifulsoup, concerning the code in question? \$\endgroup\$ – janos Dec 14 '15 at 14:13
  • \$\begingroup\$ I did not mean to say that scrapy is better, simply that it's worth taking a look at, even if to gain another perspective of operation. \$\endgroup\$ – Quill Dec 14 '15 at 14:14
3
\$\begingroup\$

get_column_index could just return None, then the test becomes if column_index:. However the bigger concern is it using the columns variable unexpectedly when it's only introduced much later in the code, i.e. not provided as a parameter. I'd change that and pass in columns explicitly because it's confusing even if the language allows it.

  • result_sets_col is being used as a constant, so it should follow constant naming convention, e.g. RESULT_SETS_COLUMN.
  • I'd introduce text = script.text to shorten it a bit.

Otherwise looks good. Unit tests are also nicely split.

\$\endgroup\$
  • \$\begingroup\$ I don't see your point about result_sets_col. It's not a constant. It's a local implementation detail. As for the sneaky usage, the language allows this, deliberately. It's a feature of functional languages to allow defining functions in local scope, and letting them access variables in their surrounding scope. In fact, adding the (redundant) column parameter results in a lint warning. Moving the get_column_index function outside doesn't really make sense, as I don't need it outside, it's an implementation detail of extract_column \$\endgroup\$ – janos Dec 14 '15 at 13:37
  • \$\begingroup\$ I didn't mean to move get_column_index outside, I agree with the implementation detail; columns is defined after the function and appears several levels deeper, so it really should be a different scope and not surrounding the function. \$\endgroup\$ – ferada Dec 14 '15 at 13:43
  • \$\begingroup\$ If I add columns as a parameter, its value will be exactly as is in the surrounding scope, and as such completely redundant. Actually I used that parameter at first, but that raised lint warnings, for shadowing the columns variable in the scope. I removed it to eliminate the warning, and because it's logical, and because I remembered examples in functional programming where this writing style is actually recommended, as opposed to redundancies. Btw you haven't addressed my response about the non-constant result_sets_col \$\endgroup\$ – janos Dec 14 '15 at 13:56
  • \$\begingroup\$ For result_sets_col it looks like it's used to define the 'resultSets' in one place, then reuse it. Even though it is not a global constant, it's certainly used as a constant here. I wanted to suggest that if it's used as a constant, it probably should be named like one. \$\endgroup\$ – ferada Dec 14 '15 at 14:59
  • 1
    \$\begingroup\$ I agree with ferada wrt result_sets_col; one other comment about it: since you're never changing it, set it outside the loop - unoptimized interpreters may not realize it can be moved outside the loops and will spend time executing it (or at least, considering executing it) every time through. \$\endgroup\$ – pjz Dec 14 '15 at 16:27
3
\$\begingroup\$

I'd test if not result_sets_col in script.text and continue in that case, to remove a level of nesting:

for script in soup.findAll('script'):
    result_sets_col = 'resultSets'
    if not result_sets_col in script.text:
        continue

    start = script.text.rindex('{', 0, script.text.index(result_sets_col))
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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