3
\$\begingroup\$

I am a beginner Python developer and I wrote a python script which reads releases data from Github for a specific release of a repository and write those release details to a specific file in google sheet. I used Github package for reading data from github API and gspread package for writing data to google sheet file. But my script is not structured well in code and has slow performance. I really like get comments about improving code structure and performance of script.

# -*- coding: utf-8 -*-
#! /user/bin/python2

# Import packages and libraries
from github import Github
from github import BadCredentialsException
import gspread_formatting
import gspread
import oauth2client
from oauth2client.service_account import ServiceAccountCredentials
import getpass


class ReadRelease:
    # Instances vairables
    sheet = ''
    username = ''
    password = ''
    repository_name = ''
    release_name = ''

    # Get user credentials from consule
    def get_user_credentials (self):
        self.username = raw_input("Github username: ")
        self.password = getpass.getpass(prompt='Github password: ')
        self.repository_name = raw_input("Repository name: ")
        self.release_name = raw_input('Release name: ')

    # Read details of all releases in a repository
    def read_requested_release_detials(self, release, tag_name, sheet, is_release_found):
        is_release_found = True
        release_body = release.body
        lines_in_release_body = (release_body.splitlines(0))
        ReadRelease.read_lines_start_with_dash(self, lines_in_release_body, self.sheet, tag_name)

    # Read all releases names of a repository
    def read_all_releases_names(self, repository, release_name, sheet):
        is_release_found = False
        for release in repository.get_releases():
            tag_name = (release.tag_name).encode('utf-8')
            name = (release.title).encode('utf-8')
            # Filter releases name with give release name
            if name == release_name:
                is_release_found = True
                ReadRelease.read_requested_release_detials(self, release, tag_name, self.sheet, is_release_found)
            if not is_release_found:    
                print "'Release Name' is wrong!, Please try again with correct 'release name'"              
        else:
            print "Releases inserted to the Google sheet."

    # Insert a line to google sheet file (those issues which are not fixed)
    def issue_not_fixed(self, sheet, tag_name, line, row):
        sheet.insert_row([tag_name, tag_name, line[7:].encode('utf-8'), 'NO'], row)

    # Insert a line to google sheet file and set fix the Is fixed? column
    def issue_fixed(self, sheet, tag_name, line, row):
        sheet.insert_row([tag_name, tag_name, line[7:].encode('utf-8'), 'YES'], row)

    # Read all lines in ralease body whihc start with '-'
    def read_lines_start_with_dash(self, lines_in_release_body, sheet, tag_name):
        # Counter for updating rows in google sheet file
        row = 2
        for line in lines_in_release_body:
            if (line[:1]).encode('utf-8') == '-':
                if line[1:7].encode('utf-8') == ' [FIX]':
                    ReadRelease.issue_fixed(self, sheet, tag_name, line, row)
                elif line[1:7].encode('utf-8') == '------':
                    break
                else:
                    ReadRelease.issue_not_fixed(self, sheet, tag_name, line, row)
            row += 1

    # Set google sheet api credentials and open file on google sheet
    def set_google_sheet_credentials(self, sheet):
        scope = ['https://spreadsheets.google.com/feeds','https://www.googleapis.com/auth/drive']
        google_credentials = ServiceAccountCredentials.from_json_keyfile_name('test.json', scope)
        file = gspread.authorize(google_credentials)
        self.sheet = file.open('Copy_of_MOD_Change_Worklog_Tracker.xlsx').sheet1
        # Add headers to the sheet
        self.sheet.update_acell('A1', 'Client Version')
        self.sheet.update_acell('B1', 'Version Reference for Internal Purposes')
        self.sheet.update_acell('C1', 'Proposed Change (Expected Functional Behavior)')
        self.sheet.update_acell('D1', 'Bug Fix?')   

    # URL Builder method for github repository
    def prepare_github_repository_url(self):
        return(str('someuseraccount/'+ self.repository_name))

    # Authunticate user in github 
    def github_authunticate(self, username, password, url):
        github_account = Github(self.username, self.password)
        return (github_account.get_repo(url))        

def main():
    read_release_obj = ReadRelease()

    read_release_obj.get_user_credentials()

    url = read_release_obj.prepare_github_repository_url()

    # Check if username and password exist
    if read_release_obj.username and read_release_obj.password: 
        try:
            github_repo = read_release_obj.github_authunticate(read_release_obj.username, read_release_obj.password, url)

            read_release_obj.set_google_sheet_credentials(read_release_obj.sheet)

            read_release_obj.read_all_releases_names(github_repo, read_release_obj.release_name, read_release_obj.sheet)

        except BadCredentialsException:
            print ('Bad credentials. Try again!')

    else:
        print ('Github credentials are required!')

if __name__ == "__main__":
    main()
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

You are using classes wrong. Your first wrong statement is actually your first comment # Instances vairables. Variables which you set at the class scope are class variables, they are in general shared by all instances of the class. Here this distinction does not really matter since you only ever have one instance of the class and when you do change them in the instance you override them with a new string instead of mutating the object there (which is impossible since strings are immutable in Python). But if you ever write a class where these objects are mutable (a list for example) you are in for a hell of a surprise:

class A:
    x = []

a1 = A()
a2 = A()
a1.x.append(2)
print(a2.x)
# [2]

The next thing where you are using classes wrong is when calling the class methods. The usual way is this:

class A:
    def some_method(self, *args):
        print("some_method", *args)

    def other_method(self):
        self.some_method(1)

Note that in the second method I did not write A.some_method(self, 1). self.some_method is automatically expanded to that (and a lot more readable).


And finally, I would question if this needs to be a class at all. Almost all parameters are explicitly passed to the methods anyways and the only state you keep around is self.sheet. But you might as well just keep that in a variable.

Without a class your code could look like this:

import getpass
import gspread
from github import Github, BadCredentialsException
from oauth2client.service_account import ServiceAccountCredentials


def get_user_credentials():
    while True:
        username = raw_input("Github username: ")
        password = getpass.getpass(prompt='Github password: ')
        if username and password:
            return username, password
        print 'Github credentials are required!'


def get_repo(username, password, repository_name):
    try:
        github_account = Github(username, password)
        return github_account.get_repo('someuseraccount/' + repository_name)
    except BadCredentialsException:
        print 'Bad credentials. Try again!'
        raise

def get_releases(repository, release_name):
    releases = [release
                for release in repository.get_releases()
                if release.title.encode('utf-8') == release_name]
    if not releases:
        raise RuntimeError("'Release Name' is wrong!, Please try again with correct 'release name'")
    return releases

def get_issues(release):
    tag_name = release.tag_name.encode('utf-8')
    for line in release.body.splitlines():
        line = line.encode('utf-8')
        if line.startswith('- [FIX]'):
            yield "YES", tag_name, line[7:]
        elif line.startswith('-' * 7):
            yield "NO", tag_name, line[7:]

def get_sheet(file_name):
    scope = ['https://spreadsheets.google.com/feeds',
             'https://www.googleapis.com/auth/drive']
    google_credentials = ServiceAccountCredentials.from_json_keyfile_name(
        'test.json', scope)
    f = gspread.authorize(google_credentials)
    sheet = f.open(file_name).sheet1
    # Add headers to the sheet
    sheet.update_acell('A1', 'Client Version')
    sheet.update_acell('B1', 'Version Reference for Internal Purposes')
    sheet.update_acell('C1', 'Proposed Change (Expected Functional Behavior)')
    sheet.update_acell('D1', 'Bug Fix?')
    sheet.resize(1)
    return sheet

def write_issues_to_sheet(sheet, issues):
    for fixed, tag_name, line in issues:
        sheet.append_row([tag_name, tag_name, line, fixed])
    print "Release inserted to the Google sheet."

def main():
    username, password = get_user_credentials()
    repository_name = raw_input("Repository name: ")
    repo = get_repo(username, password, repository_name)
    sheet = get_sheet('Copy_of_MOD_Change_Worklog_Tracker.xlsx')

    release_name = raw_input('Release name: ')
    for release in get_releases(repo, release_name):
        write_issues_to_sheet(sheet, get_issues(release))

if __name__ == "__main__":
    main()

I also moved around a bit where stuff is being done in order to decouple the functions from each other. Now they are all being called from the main function which handles passing them the right parameters. This drastically reduced the number of parameters needed by the functions, since now they don't need to take the parameters of the functions they are going to call inside.

I also used sheet.append_row which appends a row after the last row. For this to work correctly we must first set the last row to be the first one (because otherwise it will add rows starting at row 1000). For this I used sheet.resize. You might have to do sheet.resize(2) instead of 1 if it overrides your header, can't test this ATM.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your nice guides and solution. Would you please tell more about impact of your changes and solution on performance of script? \$\endgroup\$ Jan 30, 2019 at 9:36
  • \$\begingroup\$ @IbrahimRahimi The script will probably not get significantly faster by changing this. It will just be easier to read and maintain. It should also be easier to figure out where the actual bottle neck is by profiling how long each function takes to execute. \$\endgroup\$
    – Graipher
    Jan 30, 2019 at 10:09

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.