Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

points: assign points for having PRs merged #33

Merged
merged 13 commits into from Jan 17, 2019
Merged

points: assign points for having PRs merged #33

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cf31162
points: assign points for having PRs merged
jeffkaufman Jan 7, 2019
b4e98f5
Merge branch 'master' into points-for-changes
jeffkaufman Jan 7, 2019
cf4b972
catch all errors, not just ValueErrors
jeffkaufman Jan 8, 2019
cc8a04f
only count points for PRs after this one
jeffkaufman Jan 8, 2019
230c8d7
Merge branch 'master' into points-for-changes
jeffkaufman Jan 9, 2019
d698e86
Merge branch 'master' into points-for-changes
jeffkaufman Jan 9, 2019
69eee07
don't assign points for #33 or any prs merged before then
jeffkaufman Jan 9, 2019
1e8c77c
support bonus directory
jeffkaufman Jan 9, 2019
b1b437c
comment
jeffkaufman Jan 9, 2019
443f146
Revert "comment"
jeffkaufman Jan 9, 2019
a6895a8
Revert "support bonus directory"
jeffkaufman Jan 9, 2019
caa687f
add comments; remove points printing
jeffkaufman Jan 9, 2019
85b29cb
Merge branch 'master' into points-for-changes
jeffkaufman Jan 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 33 additions & 0 deletions validate.py
View file
Open in desktop
@@ -1,5 +1,6 @@
import os
import random
import re
import requests
import subprocess
import time
Expand Down Expand Up @@ -85,6 +86,34 @@ def get_reviews():
def get_users():
return list(sorted(os.listdir('players/')))

def get_user_points():
points = {}

for user in get_users():
points[user] = 0

with open(os.path.join('players', user)) as inf:
try:
points[user] += int(inf.read())
except ValueError:
jeffkaufman marked this conversation as resolved.
Show resolved Hide resolved
pass

cmd = ['git', 'log', 'master', '--first-parent', '--format=%s']
completed_process = subprocess.run(cmd, stdout=subprocess.PIPE)
if completed_process.returncode != 0:
raise Exception(completed_process)
process_output = completed_process.stdout.decode('utf-8')

merge_regexp = '^Merge pull request #[\\d]* from ([^/]*)/'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dangerous way of determining whether something is a merge commit or not. Someone could fill a PR with many commits, any one of which could contain a message that matches this that someone might not notice.

Almost makes me wonder if this is an intentional oversight :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the --first-parent history (see @dchudz 's http://www.davidchudzicki.com/posts/first-parent/), your comment would be correct if that weren't included. See:

[master] $ git log --format=%s | head -n 10
maintenance: make all clones include full repo histories
Merge pull request #26 from jeffkaufman/rework-players
Merge pull request #23 from jeffkaufman/lower-threshold
make it clearer that last_commit_ts is about master
consider rejections at previous commits
don't allow merging if any rejections
players: switch to a directory
remove debugging code
approval-threshold: lower it daily after three days with no commits
Merge pull request #6 from TheJhyde/master

[master] $ git log --first-parent --format=%s | head -n 10
maintenance: make all clones include full repo histories
Merge pull request #26 from jeffkaufman/rework-players
Merge pull request #23 from jeffkaufman/lower-threshold
Merge pull request #6 from TheJhyde/master
Merge pull request #9 from jeffkaufman/dchudz-patch-1
Merge pull request #10 from jeffkaufman/limit-non-players
Merge pull request #7 from dchudz/patch-1
add debugging information for approval commit checking
Merge pull request #8 from jeffkaufman/jeffkaufman-patch-1
tell validate which scenario it's running in

In the first case, we have commits like "consider rejections at previous commits" (caad3e7) which isn't part of a PR. In the second only the merge commits (and things I directly pushed to master in my admin role) are listed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Learn something new every day!

for commit_subject in process_output.split('\n'):
match = re.match(merge_regexp, commit_subject)
if match:
commit_username, = match.groups()
if commit_username in points:
points[commit_username] += 1
jeffkaufman marked this conversation as resolved.
Show resolved Hide resolved

return points

def last_commit_ts():
# When was the last commit on master?
cmd = ['git', 'log', 'master', '-1', '--format=%ct']
Expand Down Expand Up @@ -150,6 +179,10 @@ def determine_if_mergeable():
print('\nPASS')

def determine_if_winner():
print('Points:')
for user, user_points in get_user_points().items():
jeffkaufman marked this conversation as resolved.
Show resolved Hide resolved
print(' %s: %s' % (user, user_points))

users = get_users()
for user in users:
if random.random() < 0.0001:
Expand Down