2
\$\begingroup\$

I've written this snippet to extract a review request status from review-board in python. It works well but i'm very new to python and i would like to know if i could write this any better.
Should i put conn.close in it's own try statement ?

This script is supposed to run as part of a post-commit hook in mercurial. If the review-board server is not responding i still want to be able to commit without any issues. If the commit is non existent then an empty string is ok.

import httplib
import json

def getReviewStatus(reviewboardUrl, id):
    try:
        conn = httplib.HTTPConnection(reviewboardUrl, 80)
        headers = {
            'Accept': 'application/json',
            'Content-Type': 'application/json; charset=UTF-8'
        }
        conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
        return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""
    finally:
        conn.close()
\$\endgroup\$
2
  • 1
    \$\begingroup\$ is this a standalone script? if so I'd let it blow on errors, don't catch any exceptions (specially don't silently catch all exceptions) \$\endgroup\$
    – tokland
    Commented May 30, 2013 at 9:45
  • \$\begingroup\$ i amended my question \$\endgroup\$ Commented May 30, 2013 at 9:52

1 Answer 1

1
\$\begingroup\$

Looks pretty good to me. Regarding conn.close(), definitely don't put this in its own try statement, because if there's a problem with the earlier code, the connection might not get closed. I know you're not using Python 3, but ss an alternative, you might consider using the with statement and let Python handle the closing for you. See http://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects (end of section) and http://docs.python.org/2/library/contextlib.html#contextlib.closing

Your code might look like this:

import httplib
import json
from contextlib import closing

def getReviewStatus(reviewboardUrl, id):
    try:
        with closing(httplib.HTTPConnection(reviewboardUrl, 80)) as conn:
            headers = {
                'Accept': 'application/json',
                'Content-Type': 'application/json; charset=UTF-8'
            }
            conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
            return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""

I find it's good to get into the habit of using with to open resources so don't have to worry about forgetting to close the connection.

\$\endgroup\$
4
  • \$\begingroup\$ Wow, this is exactly what i had in mind ! Thanks a bunch !! i knew about with but i didn't knew about from contextlib import closing, absolutely brilliant ! \$\endgroup\$ Commented May 30, 2013 at 17:35
  • 1
    \$\begingroup\$ Out of interest, what would happen with this code if the requests throws an exception and the implicit close call throws an exception as well ? \$\endgroup\$ Commented May 30, 2013 at 17:39
  • \$\begingroup\$ That's a good question! This would be difficult to test. Maybe ask on Stack Overflow? I personally have never seen an exception thrown while trying to close a file/socket. Are you getting one, or just trying to code defensively? \$\endgroup\$
    – Johntron
    Commented May 30, 2013 at 18:01
  • \$\begingroup\$ just trying to be defensive :) \$\endgroup\$ Commented May 30, 2013 at 22:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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