Get Wikimedia attributions for images

Question

We are using images from Wikimedia commons for some of Cardshifter's game artwork. I selected some art for each card to make a .jpg file to use in the game clients, and added a URL to the original file as a comment for reference. This is what one card looks like: (with our DSL made from Groovy)

// https://en.wikipedia.org/wiki/Sun_Wukong
// https://commons.wikimedia.org/wiki/File:Sun_Wukong_and_Jade_Rabbit.jpg
card('MONKING') {
    creature 'Chinese Hero'
    flavor 'Monkey King Warrior of Immense Strength.'
    imagePath 'mythos/chinese/monking.jpg'
    maxInDeck 2
    health 5
    sickness 0
    manaCost 15
    attack 5
    denyCounterAttack()
    onEndOfTurn {
        change ATTACK by 1 on {
            thisCard()
        }
    }
}

However, the problem I was facing is that while I was gathering images, I forgot to also get the attribution information for each image. So, I wrote a Python script and a few helper classes to do this work.

The script uses Selenium webdriver to control Firefox (as suggested by @jacwah), along with some read & write instructions and a bit of regex to isolate URLs to be used by Selenium to automate getting the information from Wikimedia.

In the resulting output files, the above card's header will look like this with the added attribution text (or an error if Selenium is unable to resolve it, by design)

// https://en.wikipedia.org/wiki/Sun_Wukong
// https://commons.wikimedia.org/wiki/File:Sun_Wukong_and_Jade_Rabbit.jpg
// Attribution: By Yoshitoshi Tsukioka (http://www.japaneseprints.net/viewitem.cfm?ID=2182) [Public domain], via Wikimedia Commons// License: Public Domain
card('MONKING') {
    creature 'Chinese Hero'
    flavor 'Monkey King Warrior of Immense Strength.'
    imagePath 'mythos/chinese/monking.jpg'
...

---

This is my first rodeo with Python, please help me improve any aspect of the code that you feel is not "Pythonic", not efficient, or just plain terrible.

Note: The script takes about 5 minutes to run, keeping in mind that the vast majority of the time is spent by Selenium opening URLs in Firefox, clicking links, etc. for probably about 80-90 cards.

---

RegexUtilityPatterns.py ^{(to keep useful patterns in one place)}

import re

URL_MATCH = re.compile(r"""
    http[s]?://     # Match protocol, secure or not
   (?:
        [a-zA-Z]|       # Any letter
        [0-9]|          # Any number
        [$-_@.&+]|      # Other allowed symbols
        [!*\(\),]|      # Exclamation, parens, commas
        (?:
            %[0-9a-fA-F][0-9a-fA-F]  # percent (%) sign followed by 2-digit hex
        )
   )
   +  # Match one or more of preceding tokens
   """, re.VERBOSE)

ListOfFilesInDirectory.py

from Debug import Debug
import os

class ListOfFilesInDirectory(object):

    @Debug()
    def __init__(self, directory, extension=".*"):
        """
        Get a list of file names from a directory.
            :param directory: Where to look for files
            :param extension: The desired file extension (optional)
            :return: list of file names as strings
            :rtype: object
        """
        self.directory = directory
        self.extension = extension

    def list_of_files(self):
        """
        Scans a directory for files, matching an optional file extension,
        or all file extensions if omitted.
        :return: list of file names as strings
        """
        directory = self.directory
        extension = self.extension
        file_list = []

        if not directory.endswith("/"):
            directory += "/"
        if not extension.startswith("."):
            extension = "." + extension
        for file_name in os.listdir(directory):
            if file_name.endswith(extension):
                file_list.append(file_name)

        return file_list

Note that Debug is a useful decorator designed by @Ethan Bierlein

WikimediaAttributionsFromFilesInDirectory.py

from ListOfFilesInDirectory import ListOfFilesInDirectory
from selenium import webdriver
import RegexUtilityPatterns
import sys
import re

source_directory = "/Users/francisveilleux-gaboury/IdeaProjects/Cardshifter/extra-resources/mods/Mythos/"
extension = ".cardset"
file_names = ListOfFilesInDirectory(source_directory, extension)
output_directory = "/Users/francisveilleux-gaboury/Downloads/"

filter_include = [
    "wikimedia"
]
filter_exclude = [
    "User:",
    "upload.wikimedia"
]
url_regex = RegexUtilityPatterns.URL_MATCH

try:
    firefox = webdriver.Firefox()
    for file_name in file_names.list_of_files():
        with open(output_directory + file_name, "w") as output:
            full_file_path = source_directory + file_name
            print "/* " + full_file_path + " */"
            output.write("// " + full_file_path)
            with open(full_file_path, "r") as source_file:
                for line in source_file:
                    url_match = re.search(url_regex, line)
                    if url_match:
                        url_list = re.findall(url_regex, line)
                        url = url_list[0]
                        if any(value in url for value in filter_include) \
                                and any(value not in url for value in filter_exclude):
                            try:
                                firefox.get(url)
                                firefox.find_element_by_css_selector("a[title=\"Use this file on the web\"]").click()
                                firefox.find_element_by_id("stockphoto_attribution_html").click()
                                attribution_text = firefox.find_element_by_id("stockphoto_attribution").get_attribute("value")
                                print(
                                    "// " + url + "\n"
                                    + "// Attribution: " + attribution_text
                                )
                                output.write(
                                    "// " + url + "\n"
                                    + "// Attribution: " + attribution_text
                                )

                            except:
                                e = sys.exc_info()[0]
                                print(
                                    "// " + url + "\n"
                                    + "// Exception: " + str(e) + "\n"
                                )
                                output.write(
                                    "// " + url + "\n"
                                    + "// Exception: " + str(e) + "\n"
                                )
                        else:
                            print("// " + url)
                            output.write("// " + url + "\n")
                    else:
                        print(line),
                        output.write(line),
finally:
    firefox.quit()

Answer 1

Python

I know you're used to Java, which is probably why you've put everything in classes in separate files. @SuperBiasedMan commented on this, but I want to stress this.

Python is not a strictly object oriented language like Java, you can mix different approaches as you see fit. ListOfFilesInDirectory is a good candidate to be a function, because it doesn't need any internal state except parameters. See Classes vs. Functions.

Object initialization vs creation

def __init__(self, directory, extension=".*"):
    """
    Get a list of file names from a directory.
        :param directory: Where to look for files
        :param extension: The desired file extension (optional)
        :return: list of file names as strings
        :rtype: object
    """

A note: __init__ doesn't return anything. This method initializes an already created object. The method that creates objects is called __new__, but there's seldom a reason to touch it.

Filename matching

Python comes with a module for filename matching called fnmatch. It supports so-called glob patterns like *.cardset, which would match any file with the .cardset extension. Example from the docs:

import fnmatch
import os

for file in os.listdir('.'):
    if fnmatch.fnmatch(file, '*.txt'):
        print file

I suggest that instead of keeping track of dots and extensions you take a fnmatch pattern as an argument to your function returning file names.

Arrow shaped code

WikimediaAttributionsFromFilesInDirectory.py is a bit hard to read due to the high level of indentation and nested loops. I think you should extract each level of the loop into its own function: handle_file(name) and handle_line.

You can also extract the print statements into a function like write_comment(str, stream) where stream is either the output file or sys.stdout (which print writes to behind-the-scenes).

get_attribution(url, driver) could also be a function that encapsulates the following logic:

firefox.get(url) 
firefox.find_element_by_css_selector("a[title=\"Use this file on the web\"]").click()
firefox.find_element_by_id("stockphoto_attribution_html").click()
attribution_text = firefox.find_element_by_id("stockphoto_attribution").get_attribute("value")

Variable naming

I think this is generally good, except for firefox. Names should explain what a variable is used for, not how it does it. I consider the fact that the web browser being emulated is Firefox an implementation detail, and something that you might want to change. Using webdriver.Chrome would probably give the exact same effects, but then the name wouldn't make sense any more. I would name the variable driver or browser.

Answer 2

This could just as easily be me being too unused to OOP but I don't see the point of your class. You only call it once, it only contains two methods and __init__ could easily be supplanted by just taking the directory and extension as parameters to list_of_files. Really I'd change it to just a function that accepts the parameters directly.

Also you can shorten your final for loop with something in Python called a list comprehension. It essentially collapses a for loop into a single line that generates a list.

for file_name in os.listdir(directory):
    if file_name.endswith(extension):
        file_list.append(file_name)

turns into

file_list = [file_name for file_name in os.listdir(directory) if file_list.endswith(extension)]

It's a bid odd syntax especially with an if statement, but it's basically var_to_add for var in iterable if condition. It accomplishes the exact same as your for loop. Though Python should have a 79 character limit per line, so I'd split it over two like this:

file_list = [file_name for file_name in os.listdir(directory)
               if file_name.endswith(extension)]

The Python style guide says that you should organise your imports neatly in a specific order.

standard library imports
related third party imports
local application/library specific imports

You actually have it the other way around it seems. And you should put a blank line between each group.

import re
import sys

import RegexUtilityPatterns
from selenium import webdriver

from ListOfFilesInDirectory import ListOfFilesInDirectory

Using concatenation to make file paths isn't recommended. Python's os module has a path joining operation you should use.

with open(os.path.join(output_directory, file_name), "w") as output:

Also you don't need to specify 'r' when opening a file, open defaults to 'r'. Though including it can make it more readable as it signals what the file is for.

You should use str.format when you want to mix string literals and variables. It's a handy syntax that would turn this:

print(
    "// " + url + "\n"
    + "// Attribution: " + attribution_text
)

Into this:

print("// {}\n// Attribution: {}".format(url, attribution_text))

You use {} in places where you want substitution to occur, then pass all the parameters at the end that will substitute into each of those points in order. There are many other helpful uses for this so it's good to be used to it. I'd also note that you use brackets to print here but not earlier. Presumably you only wanted them here because it's a multiline expression but you should use brackets everywhere for consistency.