Commit repo / series to run through code review tools

gistfile1.md

Goals

• At least some changes should be of sufficient size to portray actual commits
• Cover all common operations / change request types

Base Repo

main.py

from file1 import method1
from file3 import method3
if __name__ == '__main__':
    method1(32)
    method3(12)

file1.py

def method1(a):
    print("1 Saw {}".format(a))

file3.py

def method3(a):
    print("3 Saw {}".format(a))

Code review scenario #1 - Basics of responding

• Edits by Joe submitted for merging

  • main.py:
    • duplicate method1 call.

• Response by Jane

  (flag line with duplicated call) I think we need a new method2 function instead of a duplicate; put its source alongside method1 in file1 though.

• Response by Joe

  • main.py

    • change duplicated (2nd) call to method2, adds import

  • file1.py

    • implement method2 (print("2 saw {}")...)

• Response by Jane

  Looks good, merge it in (+1)

Code review scenario #2 - Adding a method / subtle, non-requested changes

• Commit by Joe

  • main.py

        (at end)
        print(quicksort([4,3,1,2,8,0]))
    

  • file2.py

        def quicksort(array, left = 0, right = -1):
            if right < 0:
                right = len(array) - 1
            if left >= right:
                return
            pivot = (left + right) // 2
    
            pv = array[pivot]
            tmp = array[pivot]
            array[pivot] = array[right]
            array[right] = tmp
            store = left
            for i in range(left, right):
                if array[i] <= pv:
                    t = array[i]
                    array[i] = array[store]
                    array[store] = t
                    store += 1
    
            t = array[store]
            array[store] = array[right]
            array[right] = t
    
            quicksort(array, left, store)
            quicksort(array, store + 1, right)
            return array
    

• Response by Jane

  It'd be faster if you added " - 1" to the right hand side of the first recursion. The pivot is already sorted!

• Response / edit by Joe

  Oh, thanks

  • file2.py, at the end; note that poor Joe skipped the word "first"

        quicksort(array, left, store - 1)
        quicksort(array, store + 1, right - 1)
    

  • file2.py as well, Joe decided to do a little clean up as well, and get rid of the unneeded tmp var up top. He is over aggressive.

        ...
        pv = array[pivot]
        array[right] = pv
        store = left
        ...
    

• Response by Jane

  No no, just the first quick sort! (Is it easy to see the first mistake has been added?) Also, you deleted an extra line (comment on it in the old version, if possible).

• Edits by Joe

  Sorry! Better?

• Response by Jane

  Better. +1

Code Review Scenario #3 - edits and file renames

• Edits by Joe (Request #1)

  • file2.py

        Rename variable "t" to "tmp" for clarity.
    

• Edits by Charlie (Separate Request #2)

  • file2.py renamed to quicksort.py

    It's a quicksort, not a nameless function!

• Jane approves requests in either order, noting what happens