7
\$\begingroup\$

I have this code which creates a team using the github API and returns the team-id(success) or -1(failure).

My problem is with the error handling. When the request responses with an error status code I try to deal with a few different types of failure. It looks like a mess!

Is there any way to make it more readable/elegant?

I'd also like to know if I am catching all the reasonable exceptions that can be thrown by requests.

def create_team(org,team_name):
    '''
    Try to create a team and return its id
    if already exisits return id
    '''
    try:
        data = {
            "name": team_name,
            "permission": "push"
        }
        headers = {'Authorization': 'token '+OAUTH_TOKEN}

        response = requests.post(GH_API_URL+'orgs/'+org['login']+'/teams',
                                    headers=headers,
                                    data = json.dumps(data))

        response.raise_for_status() #throw exception if request does not retun 2xx

        #http status is 2xx, team must have been created
        json_response = json.loads(response.text)
        return json_response['id'] #get id of new team from response

    except requests.exceptions.HTTPError as e:

        if response.status_code == 422: #Unprocessable Entity 
            json_response = json.loads(response.text)
            if json_response['errors'][0]['code'] == 'already_exists': #Unprocessable because already exists
                print(' not created, team already exists!!!')

                #get the id of existing team
                team_id = get_teamID_from_name(org, team_name)

                return team_id
            else:  #Unprocessable for some other reason
                print(' HTTP error: '+str(e))

        else: #other HTTP error != 422
            print(' HTTP error: '+str(e))

    except requests.exceptions.RequestException as e:
        print('Connection error: '+str(e))



    return -1

In case you were wondering why I don't just check for the existence of the team BEFORE trying to create it. That's because the normal case for this function is that the teams do not already exist and the github API is quite slow. As it stands it takes several minutes to process a groups with 50 teams and 100 students. Checking for team existence would add a few more minutes to the runtime.

\$\endgroup\$
  • 1
    \$\begingroup\$ Where does the various constants (as well as IssueLog) come from? \$\endgroup\$ – 409_Conflict Feb 28 '17 at 11:35
  • \$\begingroup\$ @MathiasEttinger they are all file level globals. Yeah, I know OAUTH_TOKEN shouldn't be global :-(. I've taken out the IssueLog for clarity \$\endgroup\$ – Ken Feb 28 '17 at 12:04
4
\$\begingroup\$

One way to slightly speed-up is by reducing the overhead of the requests module. Currently your code has to establish a connection once for every team, re-transmitting the authentication API code and so on. This is a solved problem, you could use sessions.

You would have to start it with a context-manager in you outer function (where you loop over all of the team-names to create) and pass the session object to the create_team function.

I also shuffled some code out of the try block, you should have only the least possible amount in there, so you can be sure what exactly raised an error. I also inserted early returns, but there are other schools of thought which prefer a single exit for functions at the cost of having more code in the try block.

I also tried including a bare bones implementation for your outer function, but yours might be slightly different, of course.

I used str.format, where applicable instead of string addition.

import requests

OAUTH_TOKEN = "XXX"
HEADERS = {'Authorization': 'token {}'.format(OAUTH_TOKEN)}
GH_API_URL = "http://github.com/XXX"


def create_teams(org, team_names):
    with requests.Session() as session:
        session.headers.update(HEADERS)
        for team_name in team_names:
            create_team(session, org['login'], team_name)


def create_team(session, org_name, team_name):
    '''
    Try to create a team and return its id.
    If it already exists return id
    '''
    data = {
        "name": team_name,
        "permission": "push"
    }
    url = '{}orgs/{}/teams'.format(GH_API_URL, org_name)
    try:
        response = session.post(url, data=json.dumps(data))
        response.raise_for_status()  # throw exception if request does not return 2xx
    except requests.exceptions.HTTPError as e:
        if response.status_code == 422:  # Unprocessable Entity
            json_response = json.loads(response.text)
            # Unprocessable because already exists
            if json_response['errors'][0]['code'] == 'already_exists':
                print('{} not created, team already exists!!!'.format(team_name))
                # get the id of existing team
                return get_teamID_from_name(session, org, team_name)
        # Unprocessable for some other reason or other HTTP error != 422
        print(' HTTP error: {}'.format(e))
        return -1

    except requests.exceptions.RequestException as e:
        print('Connection error: {}'.format(e))
        return -1

    # http status is 2xx, team must have been created
    json_response = json.loads(response.text)
    return json_response['id']  # get id of new team from response
\$\endgroup\$
  • \$\begingroup\$ sessions is new to me thanks, that'll definitely make the entire app a bit tidier, and hopefully a bit quicker \$\endgroup\$ – Ken Feb 28 '17 at 16:00

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.