# Pair programming matrix

At work, we have a "pair programming ladder" where you can keep track of who is pairing with whom (based on check-ins). The idea is to promote "promiscuous pairing" where each developer eventually pairs with every other developer.

In our team, this was being updated manually, so I thought of writing a script that will parse the logs and generate a table.

Given these are the names of the developers:

"Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"

The output would look like so:

       |Adam
Bill   |0      |Bill
Carl   |0      |2      |Carl
David  |1      |0      |0      |David
Earl   |0      |0      |3      |0      |Earl
Frank  |0      |0      |0      |2      |1      |Frank
George |0      |0      |0      |0      |2      |0      |George
Solo   |1      |0      |0      |0      |0      |1      |0      |Solo
Other  |1      |0      |0      |0      |0      |2      |1      |0      |Other


This is my script:

from git import *
import re
from collections import Counter

list = ["Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"]

def build():
repo = Repo("/home/developer/work/commerce-git/commerce-project")
pairsDict = {}
for commit in repo.iter_commits(master, since='2013-02-06+14:00', reverse=True):
for person in list:
if person not in pairsDict:
pairsDict[person] = Counter()
if person in commit.message:
pairedWith = ""

m = re.search('(\w+)/(\w+)\s', commit.message)
if not m:
pairedWith = "Solo"
else:
if m.group(1) == person:
pairedWith = m.group(2) #pair is right of slash
else:
pairedWith = m.group(1) #pair is left of slash
if pairedWith not in list:
pairedWith = "Other"
pairsDict[person][pairedWith] += 1
if pairedWith not in pairsDict:
pairsDict[pairedWith] = Counter()
pairsDict[pairedWith][person] += 1
break;

return pairsDict

def print_table():
pairsList = list + ["Solo", "Other"]
pairsDict = build()
str = ""
for index,person in enumerate(pairsList):
if (index == 0):
str += '|'.rjust(8) + person.ljust(7) + '\n'
else:
str += person.ljust(7) + '|'
for paired in pairsList[:index]:
count  = pairsDict[person][paired]
str += '{:<7}|'.format(count)
str +=  person.ljust(7)+'\n'
print str
return str

if __name__ == "__main__":
print_table();


The scripts parses the git logs using GitPython, where a commit message is expected to look like this:

"Adam/David - Refactored app to be more polyglot-y"

From there it constructs a HashMap of Counters to keep track of who has paired with whom. I'm going to eventually feed this data structure into a simple Django web-app so people can view it in a browser.

Do you have any suggestions for this script? Could it be more "pythonic"? Is there anything I could do to refactor it to be simpler to read, or better performance-wise?

-

1. No docstrings! What do these functions do and what kind of values do they return?

2. Python statements don't need to be terminated with a semicolon: they finish at the end of a line.

3. Is it really necessary to do from git import * given that you only actually use git.Repo?

4. There's a built-in function called list, so it's a bad idea to use that as the name of a variable. Also, list is a vague name: list of what? I suggest something like users.

5. Similarly, build is a vague name. Build what? I suggest something descriptive like count_commit_pairings.

6. It's conventional (following PEP8) for Python variables to use lower case with underscores. If you plan to collaborate with other Python programmers, use variable names like pairs_dict and paired_with instead of pairsDict and pairedWith.

7. Constants like '2013-02-06+14:00' shouldn't be buried deep in the code, but should be pulled out to become global variables or function parameters.

8. You could avoid having to write these lines:

if person not in pairsDict:
pairsDict[person] = Counter()


and these lines:

if pairedWith not in pairsDict:
pairsDict[pairedWith] = Counter()


if you made pairsDict into a defaultdict(Counter). See collections.defaultdict.

9. Since you're going to be looking up users to see whether they need to be replaced with 'Other', it's good practice to convert the list of users into a set, so that looking up a name can be done in constant time.

10. It's wasteful to iterate over the users and try to match each one against the commit message. Why not do a regular expression match against the commit message just once and then figure out which users are mentioned?

11. You use the line

if person in commit.message:


to determine if person is the committer. But what if someone makes a commit without writing their name in the commit message (false negative)? What if someone's name appears in the commit message by coincidence (false positive)? It seems to me that looking at commit.committer.name would be more reliable.

12. Backslash is an escape character in Python strings, so you should generally use the r modifier in front of strings where you want to preserve the backslashes. So write:

m = re.search(r'(\w+)/(\w+)\s', commit.message)

13. It's better to use the regular expression \b (word boundary) rather than \s (whitespace) to detect the end of a word. This is because \b matches if the word is followed by punctuation, or by the end of the string, whereas \s would fail in those cases.

14. If the name of the pairing is supposed to be at the start of the commit message, you should use re.match to ensure this. But if it is allowed to be anywhere in the commit message you should write:

m = re.search(r'\b(\w+)/(\w+)\b', commit.message)


to ensure that the match starts at the beginning of a word.

15. Each time you call re.search it has to compile the regular expression. Since you're going to do this for each commit, it makes sense to compile the regular expression just once by calling re.compile.

16. In the section of code

if m.group(1) == person:
pairedWith = m.group(2) #pair is right of slash
else:
pairedWith = m.group(1) #pair is left of slash


you assume that if person is in the message, and not to the left of the slash, then they must be to the right of the slash. But that doesn't follow at all. Suppose Gabrielle commits a change with the message

Gabrielle/Frank -- fixed bug 123 found by Alice


it looks to me as though your code will incorrectly deduce that this is a change committed by Alice and Frank. So I think it would be worth taking more care to detect (and perhaps report?) errors in the commit messages. People make mistakes!

### 2. Revised build function

Addressing all the issues discussed above yields the following code. But beware: I haven't tested this, so most likely there are mistakes.

from git import Repo
import re
from collections import Counter, defaultdict
import sys

def count_commit_pairings(repository = '/home/developer/work/commerce-git/commerce-project',
since = '2013-02-06+14:00',
users = 'Alice Bill Clare David Emma Frank Gabrielle'.split()):
"""
Count the number of times each pair of users has made a commit.

repository is the git repository to examine.
since is the date at which to start looking at the commits.
users is a list of users to count commits for.

Return the counts in the form of a dictionary mapping user name to
a dictionary mapping partner name to count of commits by that
pair. Commits not made by a pair are filed as if by the committer
and the fake user 'Solo'. User names not found in users are
replaced by 'Other'.
"""
repo = Repo(repository)
pairs = defaultdict(Counter)
users = set(users)
pair_re = re.compile(r'(\w+)/(\w+)\b')

"""Return the username to use for name."""
return name if name in users else 'Other'

def partner(commit):
"""Return the partner of the committer, or 'Solo' if none found."""
m = pair_re.match(commit.message)
if not m:
return 'Solo'
if commit.committer.name not in m.groups():
sys.stderr.write("Bad commit {0.hexsha}: committer {0.committer.name} "
"missing from pair {1}."
.format(commit, m.group(0)))
partner = set(m.groups()).difference([commit.committer.name])
if len(partner) != 1:
sys.stderr.write("Bad commit {0.hexsha}: duplicate pair {1}."
.format(commit, m.group(0)))
return 'Solo'

for commit in repo.iter_commits(repo.heads.master, since=since, reverse=True):
u2 = partner(commit)
pairs[u1][u2] += 1
pairs[u2][u1] += 1

return pairs


(This seems like plenty for one answer. Maybe some other person can look at print_table.)

-
Regarding print_table, that was more of a 'test method' for me, so I'm not as concerned about that. I've taken suggestions from both answers, but your answer is more thorough and simplified my code a lot. Thanks! –  Atif Feb 18 '13 at 21:36
from git import *


Generally import from * is frowned upon as it makes it more difficult to figure out where stuff comes from

import re
from collections import Counter

list = ["Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"]


Don't names things after types. Global constants should also be in ALL_CAPS. So this should probably be DEVELOPERS = [...]

def build():
repo = Repo("/home/developer/work/commerce-git/commerce-project")


Putting an absolute path to the repository is probably not all that great. It makes more assumptions about the setup of the machine than we'd like.

    master = repo.heads.master
pairsDict = {}


Python convention is lowercase_with_underscores for local names

    for commit in repo.iter_commits(master, since='2013-02-06+14:00', reverse=True):
for person in list:
if person not in pairsDict:
pairsDict[person] = Counter()


Use pairsDict = collections.defaultdict(Counter) so you don't need to do this. Alternately use a two element tuple as the key.

            if person in commit.message:


Shouldn't you do something like look at the username recorded by git as to who did the comment rather then checking the commit message?

                pairedWith = ""


Don't assign variables with meaningless content if you are just going to reassign it later

                m = re.search('(\w+)/(\w+)\s', commit.message)


This is an okay use of a single letter variable name, but I'd still call it match.

                if not m:
pairedWith = "Solo"
else:
if m.group(1) == person:
pairedWith = m.group(2) #pair is right of slash
else:
pairedWith = m.group(1) #pair is left of slash


This would be simpler if you didn't iterate over the list the developers. Just figure out who the developers were by regexing the message.

                    if pairedWith not in list:
pairedWith = "Other"
pairsDict[person][pairedWith] += 1
if pairedWith not in pairsDict:
pairsDict[pairedWith] = Counter()
pairsDict[pairedWith][person] += 1
break;

return pairsDict

def print_table():
pairsList = list + ["Solo", "Other"]
pairsDict = build()
str = ""
for index,person in enumerate(pairsList):
if (index == 0):
str += '|'.rjust(8) + person.ljust(7) + '\n'


Don't build up strings by adding, that's inefficent. You are better off just printing here

        else:
str += person.ljust(7) + '|'
for paired in pairsList[:index]:
count  = pairsDict[person][paired]
str += '{:<7}|'.format(count)
str +=  person.ljust(7)+'\n'
print str
return str


Don't both return a print. Either return the string and let the caller print or print the string and return nothing.

if __name__ == "__main__":
print_table();

-
Thanks for the review! As for "Shouldn't you do something like look at the username recorded by git as to who did the comment rather then checking the commit message?" I want to the filter the commit messages from my team specifically, as other teams commit to the same branch. –  Atif Feb 18 '13 at 19:31
@Atif, but it would still seem less error prone to check the username against a list your team's usernames –  Winston Ewert Feb 18 '13 at 19:58