Simple Python Flash app using Wikidot API to transfer pages between two wikis

Question

Here's a simple Python Flask app. It uses the Wikidot API to transfer pages between two wikis. I'd like to hear your input on the best way to refactor this code. Here are a couple of questions I have in addition to any criticisms you may have:

get_lines(): there are two places in here where exceptions can be thrown. I want to be able to catch them independently to be able to give better feedback (see how flash() is used).

However, now you have two types of returns for this function (and, in my opinion, that's not very Pythonic): you have a None type if there was an error and a variable if there wasn't. How should the exceptions be best handled here: at the call site or in the function?

def get_lines(admin):
   try:
      data = admin.pages.get_one({'site': ADMIN_SITE, 'page': SOURCE_PAGE})['content']
   except:
      flash('Error: unable to fetch the production-wiki page with mappings')
      return
   data = data.splitlines()
   try:
      idx = data.index(SEPERATOR)
      data = data[idx+1:]
   except:
      flash('Error: malformed page mapping')
      return
   return data

What's the best way to abstract update_pictures and update_wiki? They're 90% the same logic.

def update_pictures():
  try:
     admin, prod = get_proxies()
  except:
     flash('Error: Unable to initialize XMLRPC connections')
     return
  data = get_lines(admin)
  if not data:
     return
  for idx, line in enumerate(data):
     if line.count('|') != 3:
        continue
     admin_page_url, admin_file_url, prod_page_url = line.split('|')
     if not admin_page_url:
        flash('Line {0}: Error: no admin page url'.format(idx))
        return
     if not admin_file_url:
        flash('Line {0}: Error: no admin file url'.format(idx))
        return
     if not prod_page_url:
        flash('Line {0}: Error: no prod page url'.format(idx))
        return

     try:
        file_ = admin.files.get_one(ADMIN_SITE, admin_page_url, admin_file_url)
     except:
        flash('Line {0]: Error: unable to fetch file {1}/{2}'.format(idx, admin_page_url, admin_file_url))
        return

     kwargs = {'site': PROD_SITE,
               'page': prod_page_url,
               'file': admin_file_url,
               'content': file_['content'],
               'revision_comment': 'Updated by wiki_copy script'}

     if file_['comment']:
        kwargs['comment'] = admin_page_content['comment']

     try:
        prod.files.save_one(kwargs)
     except:
        flash('Line {0}: Error: unable to save page {1}/{2}->{3}/{2}'.format(idx, admin_page_url, admin_file_url, prod_page_url))
        return
     flash('Line {0}: Successful copy: {1}/{2}->{3}/{2}'.format(idx, admin_page_url, admin_file_url, prod_page_url))

def update_wiki():
  try:
     admin, prod = get_proxies()
  except:
     flash('Error: Unable to initialize XMLRPC connections')
     return
  data = get_lines(admin)
  if not data:
     return
  for idx, line in enumerate(data):
     if line.count('|') != 1:
        continue
     admin_page_url, prod_page_url = line.split('|')
     if not admin_page_url:
        flash('Line {0}: Error: no admin page url'.format(idx))
        return
     if not prod_page_url:
        flash('Line {0}: Error: no prod page url'.format(idx))
        return
     try:
        admin_page_content = admin.pages.get_one({'site': ADMIN_SITE, 'page': admin_page_url})
     except:
        flash('Line {0]: Error: unable to fetch page {1}'.format(idx, admin_page_url))
        return

     kwargs = {'site': PROD_SITE,
               'page': prod_page_url,
               'revision_comment': 'Updated by wiki_copy script'}

     title = admin_page_content['title']
     if not title:
        title = prod_page_url
     kwargs['title'] = title

     content = admin_page_content['content']
     if not content:
        flash('Line {0}: Error: page {1} has no content'.format(idx, admin_page_url))
        return
     kwargs['content'] = content

     if admin_page_content['tags']:
        kwargs['tags'] = admin_page_content['tags']

     try:
        prod.pages.save_one(kwargs)
     except:
        flash('Line {0}: Error: unable to save page {1}->{2}'.format(idx, admin_page_url, prod_page_url))
        return
     flash('Line {0}: Successful copy: {1}->{2}'.format(idx, admin_page_url, prod_page_url))
  return

I do understand that the best way to do the I/O is to do just about anything anything other than have it block the main thread and the Flask request. However, this is just a quick script (hence the use of Flask). The reason why there are two paths (one for pages, one for file attachments) is because I want to reduce the chance of timeouts.

Answer 1

def get_lines(admin):
   try:
      data = admin.pages.get_one({'site': ADMIN_SITE, 'page': SOURCE_PAGE})['content']
   except:
      flash('Error: unable to fetch the production-wiki page with mappings')
      return

You should definitely throw an exception rather than flash here. My approach is to define a UserError exception. When those are caught they are displayed to the user, other exceptions aren't. You also shouldn't catch all exceptions here. That'll make it really difficult to figure out what's going on if there is an exception here. Just catch the one exception you actually are interested in. Also, make the error message better. Have it include the SOURCE_PAGE and ADMIN_SITE. The point is to make it easy for the user to fix the problem from the error.

   data = data.splitlines()
   try:
      idx = data.index(SEPERATOR)
      data = data[idx+1:]
   except:
      flash('Error: malformed page mapping')
      return

Make this error message better. Say that it couldn't find the expected SEPERATOR, not simply malformed page mapping

   return data


def update_pictures():
  try:
     admin, prod = get_proxies()
  except:
     flash('Error: Unable to initialize XMLRPC connections')
     return

get_proxies should be the one raising the exception here.

  data = get_lines(admin)
  if not data:
     return
  for idx, line in enumerate(data):
     if line.count('|') != 3:
        continue

Shouldn't that be 2? thing1|thing2|thing3 -- 2 |'s

     admin_page_url, admin_file_url, prod_page_url = line.split('|')


     if not admin_page_url:
        flash('Line {0}: Error: no admin page url'.format(idx))
        return
     if not admin_file_url:
        flash('Line {0}: Error: no admin file url'.format(idx))
        return
     if not prod_page_url:
        flash('Line {0}: Error: no prod page url'.format(idx))
        return

Do those checks really help you? Presumably blank urls will fail in the next step anyways.

     try:
        file_ = admin.files.get_one(ADMIN_SITE, admin_page_url, admin_file_url)
     except:
        flash('Line {0]: Error: unable to fetch file {1}/{2}'.format(idx, admin_page_url, admin_file_url))
        return

You've got a typo {0] instead of {0}. M

     kwargs = {'site': PROD_SITE,
               'page': prod_page_url,
               'file': admin_file_url,
               'content': file_['content'],
               'revision_comment': 'Updated by wiki_copy script'}

     if file_['comment']:
        kwargs['comment'] = admin_page_content['comment']

kwargs usually refers to the ** arguments from a function.

     try:
        prod.files.save_one(kwargs)
     except:
        flash('Line {0}: Error: unable to save page {1}/{2}->{3}/{2}'.format(idx, admin_page_url, admin_file_url, prod_page_url))
        return

Again, don't generically catch things and ignore them. In general, its a bad idea to give feedback when unexpected errors occour. It might give an attack information they shouldn't have.

     flash('Line {0}: Successful copy: {1}/{2}->{3}/{2}'.format(idx, admin_page_url, admin_file_url, prod_page_url))

As for refactoring, I'd define a function like:

def extract_urls(admin, number):
    for idx, line in get_lines(admin):
        if line.count('|') == number:
            yield idx, + line.split('|')

Answer 2

The only one thing which looks dangerous
With this:

except:

This means you will handle all the exceptions. That is wrong.
You are supposed to handle only those exceptions which you expect.

except MyExpectedException, OtherExpectedException:

If you get unexpected exception, script has to raise this exception and crash.
This will save your time on debugging.
I see you have logs, but I recommend you to avoid such situations.