Magpi Magazine Downloader

Question

I have created a simple program at https://github.com/Epic0ne/magpi-downloader in Python which downloades all issues of the MagPi magazine by parsing this web page for links ending in .pdf, then downloading them using the urllib module. It also supports command line options, using the argparse module:

$ ./magpi_downloader.py -h
usage: magpi_downloader.py [-h] [-q] [-r] [--view] [-t FILETYPE] [-a] [-i]
                           [-e]
                           DIR [REMOTE_DIR]

Download issues of the MagPi magazine

positional arguments:
  DIR               The directory to install into.
  REMOTE_DIR        The directory to fetch from. Files must be links on that
                    page. Works best with Apache servers with the default
                    directory listing. Default: http://www.raspberrypi.org
                    /magpi-issues/

optional arguments:
  -h, --help        show this help message and exit
  -q, --quiet       Silence progress output
  -r, --reinstall   Reinstall all issues
  --view            List the issues available for install
  -t FILETYPE       The extension of the files to download. Default: pdf
  -a, --all         Install all files with the right extension.
  -i, --issues      Install the regular issues (default behavior)
  -e, --essentials  Install the 'Essentials' collection

I am looking for ways to improve the user interface, the speed, and making it generally cleaner and more pythonic.

Source code (also on GitHub):

import urllib, os, re, time, sys, argparse
from bs4 import BeautifulSoup

ISSUES_REGEX = u'MagPi[0-9]+'
ESSENTIALS_REGEX = u'Essentials_.*'

def get_installed(directory):
    if not os.path.exists(directory):
        if not quiet:
            print("Directory doesn't exist\nCreating new directory with the name {}".format(directory))
        os.mkdir(directory)
    for filename in os.listdir(directory):
        if re.match(regex, filename):
            yield filename

def download(filename, localfilename):
    localfile = get_full_path(download_dir, localfilename)
    try:
        open(localfile, 'w').close()
        urllib.request.urlretrieve(filename, localfile)
    except KeyboardInterrupt:
        print('Cleaning up...')
        os.remove(localfile)
        sys.exit()

def webopen(page):
    return urllib.request.urlopen(page)

def get_links(soup):
    for link in soup.find_all('a'):
        yield link.get('href')

def get_issues(soup):
    links = list(get_links(soup))
    for link in links:
        if re.match(regex, link):
            yield link

def get_missing(installed, all_issues):
    for issue in all_issues:
        if not issue in installed:
            yield issue

def get_full_path(directory, filename):
    return os.path.join(directory, filename)

def to_install_info(directory):
    page = webopen(remote_dir)
    soup = BeautifulSoup(page)

    issues = list(get_issues(soup))
    installed = list(get_installed(directory))
    missing = list(get_missing(installed, issues))
    return issues, installed, missing

def install(missing):
    for issue in missing:
        print('Downloading {} '.format(issue))
        download(get_full_path(remote_dir, issue), issue)
        print('Done')

def install_quiet(missing):
    for issue in missing:
        download(get_full_path(remote_dir, issue), issue)

def print_to_install_info(issues, installed, missing):
    print('{} Released:\n\n{}\n\n{} Installed:\n\n{}\n\n{} To install:\n\n{}'.format(len(issues), issues, len(installed), installed, len(missing), missing))

def install_all(missing):
    install(missing)

def install_all_quiet(missing):
    install_quiet(missing)

if __name__ == '__main__':
    parser = argparse.ArgumentParser(description="Download issues of the MagPi magazine")
    parser.add_argument('-q', '--quiet', action='store_true', help="Silence progress output")
    parser.add_argument('-r', '--reinstall', action='store_true', help="Reinstall all issues")
    parser.add_argument('--view', action='store_true', help="List the issues available for install")
    parser.add_argument('-t',  dest='filetype', metavar='FILETYPE', type=str, action='store',
            default=u'pdf', help="The extension of the files to download. Default: %(default)s")
    parser.add_argument('-a', '--all', dest='types', action='store_const', const=u'.*',
            help="Install all files with the right extension.")
    parser.add_argument('-i', '--issues', dest='types', action='store_const', const=ISSUES_REGEX,
            help='Install the regular issues (default behavior)')
    parser.add_argument('-e', '--essentials', dest='types', action='store_const',
            const=ESSENTIALS_REGEX, help="Install the 'Essentials' collection")
    parser.add_argument('directory',  metavar='DIR', type=str,
            help="The directory to install into.")
    parser.add_argument('remote_dir',  metavar='REMOTE_DIR', type=str, nargs='?',
            default='http://www.raspberrypi.org/magpi-issues/',
            help="The directory to fetch from. Files must be links on that page. Works \
                    best with Apache servers with the default directory listing. Default: %(default)s")

    args = parser.parse_args()
    download_dir = args.directory
    remote_dir = args.remote_dir
    types = args.types

    if not types:
        types = ISSUES_REGEX
    regex = u"^{}\.{}$".format(types, args.filetype)
    print(regex)
    quiet = args.quiet
    reinstall = args.reinstall
    view = args.view

    issues, installed, missing = to_install_info(download_dir)

    if reinstall:
        missing = issues
        msg = 'Overwriting issues'
    else: msg = 'Installing Issues'

    if not quiet:
        print_to_install_info(issues, installed, missing) 
        if view:
            print('To install, run without the view flag.')
        else:
            print(msg)
            install_all(missing)
    else:
install_all_quiet(missing)

I am sorry for the extreme lack of comments and docstrings, so that is what I will focus on.

Usage cases:

To download everything:

$ ./magpi_downloader.py -a download_dir

To download all regular issues:

$ ./magpi_downloader.py download_dir

To download the MagPi essentials:

$ ./magpi_downloader.py -e download_dir

To view but not install available issues:

$ ./magpi_downloader.py download_dir

Other options are as documented above.

Answer 1

The code overall is not readable. It's quite lengthy, there is no modular structure, no meaningful comments and docstrings. The first thing I would do is to split it into multiple modules grouped logically, define the docstrings for each of the functions (which may actually lead to some functions being joined together or even removed).

Also, some of the code blocks can be extracted into separate functions. For instance, apply the "Extract Method" refactoring method and extract the "parsing arguments with argparse" part into a separate function.

Now, let's go over the issues one by one:

• import organization - avoid importing multiple built-in modules on a single line. Third-party imports need to have a newline before them. Put 2 newlines after all the imports (PEP8 import guidelines)

• since you are issuing multiple requests to the same domain, I would use requests module maintaining a web-scraping session via requests.Session - this might result into a significant performance boost:

  So if you're making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase

• when instantiating a "soup" object, it's highly recommended to explicitly specify the underlying parser to avoid letting BeautifulSoup do this automatically - it might choose html.parser on your machine, but on the other machine, it may pick lxml or html5lib which may result into different parsing results (see Differences between parsers):

  soup = BeautifulSoup(page, "html.parser")
  # soup = BeautifulSoup(page, "html5lib")
  # soup = BeautifulSoup(page, "lxml")
  

• you can improve the way you locate the elements on a page. Currently, you are finding all the links via find_all("a") and then applying a regular expression to the href attribute values. BeautifulSoup can do both in a single "find" command:

  soup.find_all("a", href=re.compile(r"expression here"))
  

• the print_to_install_info() can benefit from using a multi-line string (to avoid multiple newline characters in the format template string)

• except for printing, the install() and install_quiet() functions really duplicate each other. Either have a single function with quiet argument, or, even better, use logging module with a configurable/controllable log level

• see if you can replace listdir() and an inner regex check with a single glob.glob() (or glob.iglob()) call.