2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Under what kinds of circumstances will these errors happen? \$\endgroup\$ Apr 22, 2013 at 1:50
  • \$\begingroup\$ Looking at this code again I think I understand the best way to handle errors - have get_lines re-throw inside of the exception handler instead of return. This lets you flash() on the inside and handle errors in a single way at the call site. \$\endgroup\$
    – ldrg
    Apr 22, 2013 at 6:50

2 Answers 2

2
\$\begingroup\$
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('|')
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I think the departure from the usual style of exception handling is justified here for UI reasons. If an unexpected exception happens the user will get a 500 error. With this they'll at least get an error which gives line context, keeps the program running and is at least somewhat helpful. \$\endgroup\$
    – ldrg
    Mar 24, 2013 at 8:18
  • \$\begingroup\$ Please at least try store the raise messages in any log file. This really will help in debugging and protecting. Because you will never know what type of error appeared, maybe crack in system? wiki.python.org/moin/HandlingExceptions \$\endgroup\$ Mar 24, 2013 at 14:36

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.