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.

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

points: assign points for having PRs merged #33

Merged
merged 13 commits into from Jan 17, 2019

Conversation

@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 7, 2019

This builds on #32, giving people points for authoring PRs that get merged. Points still don't affect your chance of winning.

Specific changes:

  • Adds a function to look up how many points everyone has.
  • If there's a number in your user file, that also gives you points. So this is compatible with #32 but doesn't depend on it.

Only PRs merged after this one count.

jeffkaufman added some commits Jan 7, 2019

points: assign points for having PRs merged
This builds on #32, giving people points for having PRs merged.  Points still don't affect your chance of winning.

Specific changes:

* Adds a function to look up how many points everyone has.
* If there's a number in your user file, that also gives you points.  So this is compatible with #32 but doesn't depend on it.
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 7, 2019

This made me realize the need for a maintenance change to make the whole repo available for inspection (#34)

Show resolved Hide resolved validate.py Outdated
Show resolved Hide resolved validate.py
validate.py Outdated
raise Exception(completed_process)
process_output = completed_process.stdout.decode('utf-8')

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

This comment has been minimized.

@pavellishin

pavellishin Jan 7, 2019

Collaborator

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 :)

This comment has been minimized.

@jeffkaufman

jeffkaufman Jan 8, 2019

Owner

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.

This comment has been minimized.

@pavellishin

pavellishin Jan 8, 2019

Collaborator

Learn something new every day!

@pavellishin
Copy link
Collaborator

pavellishin left a comment

Rejecting on the basis that we don't see what the results of validate.py in this PR would be in Travis; see my comment

@tnelling
Copy link
Collaborator

tnelling left a comment

Rejecting for reasons raised by other reviewers.

But also, I'm not convinced that only those merging PRs deserve credit for them, or that all PRs should be treated as equally valuable. And there could be a race to be the one who pushes the merge button, which I'm not interested in trying to win just for scoring reasons.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 8, 2019

As a corollary to what I just said, I believe the original nomic rules (or at least the ones I played under way back) specified points based on voting for or against successful rule changes, as well as for being the one to propose them. The latter bit about proposals relied on the notion of turns; I could see the question of who proposed a PR becoming ambiguous in some cases (e.g. where someone submits a PR, someone else forks it, and the fork gets merged). But approval/rejection wouldn't be hard to measure.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 8, 2019

@tnelling

I'm not convinced that only those merging PRs deserve credit for them, and there could be a race to be the one who pushes the merge button

This proposal gives points to the author of the PR, not the person who clicks "merge".

specified points based on voting for or against successful rule changes, as well as for being the one to propose them

Giving points for voting is also possible, just a bunch more work. I'd be happy to review a PR adding that.

@pavellishin
Copy link
Collaborator

pavellishin left a comment

My concerns have been allayed; this seems like a fine first step for giving people points.

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 8, 2019

Hm, if I click on "dismiss review" on @tnelling's rejection, does that just make it go away? Can I actually un-veto PRs?

image

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 8, 2019

@pavellishin

I click on "dismiss review" on @tnelling's rejection, does that just make it go away? Can I actually un-veto PRs?

Looking at the API response, that seems to change it from CHANGES_REQUESTED to DISMISSED. Luckily I wrote validate.py defensively, so APPROVED counts as an approval, and anything else (aside from COMMENTED) counts as rejection. So no, this doesn't un-veto the PR.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 8, 2019

@TheJhyde TheJhyde added the Points label Jan 9, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@tnelling have I addressed your concerns?

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 9, 2019

Not entirely. You're checking the merge number, rather than the merge time, which means that there will be points allocated that probably shouldn't be, and (potentially) points not allocated that probably should be.

I think you'd need to pull the merge time for 33 (as it doesn't exist yet), then check against that, to fulfill what I have in mind.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@tnelling you're saying you'd like to see points only for PRs merged after this one, instead of created after this one?

(I don't care either way, happy to make the change, just don't want to make it if that's not what you're asking for.)

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 9, 2019

Yup, that's what I'm saying, sorry for the lack of clarity.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 9, 2019

Also, if #32 goes in, this should be updated to grab the point totals from the players directory.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@tnelling concerns addressed in 69eee07. Now once we get to PR #33 when looking back through the history we stop and don't consider any older ones.

if #32 goes in, this should be updated to grab the point totals from the players directory.

This PR already did that, in anticipation.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

Status: this now only adds a get_user_points function which isn't called. Printing points and reading from points directories (#47) has been moved to #50.

@TheJhyde

This comment has been minimized.

Copy link
Collaborator

TheJhyde commented Jan 10, 2019

Just to clarify (IIUC): This is iterating through all commits in reverse chronological order of when they were merged into master stopping when it reaches PR #33. In other words, it is giving a point to everyone who merges a PR after Jeff merges this PR.

What, don't I deserve points for my hard work of keeping track of which PRs are the correct number of approvals, rerunning the tests, and merging? That stuff has to be done by hand.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 10, 2019

That stuff has to be done by hand.

Does it? What about something like:

import os
import random
import requests
import subprocess
import time
from collections import defaultdict

def request(url):
  request_headers = {'User-Agent': 'jeffkaufman/nomic'}
  response = requests.get(url, headers=request_headers)

  if response.status_code != 200:
    print('   > %s' % response.content)

  response.raise_for_status()
  return response

def get_repo():
  return 'jeffkaufman/nomic'

def base_url():
  return 'https://www.jefftk.com/nomic-github/repos/%s' % get_repo()

def pulls_url():
  return '%s/pulls' % base_url()

def pr_url(n):
  return '%s/%s' % (pulls_url(), n)

def reviews_url(n):
  return '%s/%s' % (pr_url(n), 'reviews')

def get_author(pr_json):
  return pr_json['user']['login']

def get_reviews(pr_n, pr_json, target_commit):
  reviews = {}

  url = reviews_url(pr_n)
  while True:
    response = request(url)

    for review in response.json():
      user = review['user']['login']
      commit = review['commit_id']
      state = review['state']

      #print('  %s: %s at %s' % (user, state, commit))
      if state == 'APPROVED' and commit != target_commit:
        # Only accept approvals for the most recent commit, but have rejections
        # last until overridden.
        continue

      if state == 'COMMENTED':
        continue  # Ignore comments.

      reviews[user] = state

    if 'next' in response.links:
      url = response.links['next']['url']
    else:
      return reviews

def get_users():
  return list(sorted(os.listdir('players/')))

def get_prs():
  pulls = {}
  url = pulls_url()
  while True:
    response = request(url)

    for pull in response.json():
      pr = {'title': pull['title'],
            'sha': pull['head']['sha'],
            'author': pull['user']['login']}

      pulls[pull['number']] = pr

    if 'next' in response.links:
      url = response.links['next']['url']
    else:
      return pulls

def start():
  for pr_n, pr in sorted(get_prs().items()):
    author = pr['author']
    print('\n%s: %s (%s):' % (pr_n, pr['title'], author))
    pr_json = request(pr_url(pr_n)).json()
    reviews = get_reviews(pr_n, pr_json, pr['sha'])
 
    if author in get_users():
      reviews[author] = 'APPROVED'

    votes = defaultdict(set)
    seen = set()
    for user, status in sorted(reviews.items()):
      vote = 'no'
      if status == 'APPROVED':
        vote = 'yes'
      votes[vote].add(user)
      seen.add(user)

    votes['tbd'] = set(get_users()) - seen

    for vote in ['no', 'yes', 'tbd']:
      vote_users = votes[vote]
      print('  %s %s: %s' % (
          vote.ljust(3), str(len(vote_users)).rjust(3),
          ' '.join('@%s' % x for x in sorted(vote_users))))

if __name__ == '__main__':
  start()
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 10, 2019

@tnelling does this meet with your approval?

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 12, 2019

Merged in master to fix merge conflicts; needs everyone's approval now: @TheJhyde @dchudz @guoruibiao @jmitchell @shaunagm @vesche @pavellishin @sligocki

@tnelling tnelling requested review from guoruibiao , shaunagm , vesche and dchudz Jan 14, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

@pavellishin this can go in if you approve it

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 17, 2019

Huh, why did our rate go from 5000 to 60?

Considering reviews at commit 85b29cba8ef785403c641a7c67a5e983ee8ef039
    > X-RateLimit-Limit: 5000
    > X-RateLimit-Remaining: 4969
    > X-RateLimit-Reset: 1547749824

...

    > X-RateLimit-Limit: 60
    > X-RateLimit-Remaining: 0
    > X-RateLimit-Reset: 1547748531
   > b'{"message":"API rate limit exceeded for 35.192.85.2. (But here\'s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://developer.github.com/v3/#rate-limiting"}'

Hm, do we not paginate through the magic URL?

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 17, 2019

Aha, yup: https://github.com/jeffkaufman/nomic/blob/master/validate.py#L93-L94

We can probably wait this out, but this should be a priority.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

Aha, yup: https://github.com/jeffkaufman/nomic/blob/master/validate.py#L93-L94

Shoot, yeah, that's going to hit any PR that has this many reviews. Travis IPs almost always have hit their limit for GitHub requests, so it's pretty unlikely this PR can merge without that being fixed.

jeffkaufman added a commit that referenced this pull request Jan 17, 2019

github api proxy: use for 'next' links
Right now PRs #33 generally fail TravisCI because once they have enough reviews that GitHub's API starts paginating they switch from my authenticating proxy with 5000 requests per hour to GitHub's default of 60.  Pagination works via the query string, so just apply the returned query string to our base url.

@jeffkaufman jeffkaufman merged commit 1ccdf66 into master Jan 17, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

Got this merged by setting a timer for when Travis said the rate limit would reset and restarting the build then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment