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

Allow validate.py to inspect the PR diff #48

Merged
merged 1 commit into from Jan 17, 2019

Conversation

Projects
None yet
6 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 9, 2019

Download the PR diff from github and parse it with unidiff. For now, just
print out what files are changed. Future PRs can use this to do things like
have different approval requirements based on what's in the diff.

Example output, for pr #47:

added:
  players/TheJhyde/bonuses/initial
  players/dchudz/bonuses/initial
  players/guoruibiao/bonuses/initial
  players/jeffkaufman/bonuses/initial
  players/jmitchell/bonuses/initial
  players/pavellishin/bonuses/initial
  players/sligocki/bonuses/initial
  players/tnelling/bonuses/initial
  players/vesche/bonuses/initial
removed:
  players/TheJhyde
  players/dchudz
  players/guoruibiao
  players/jeffkaufman
  players/jmitchell
  players/pavellishin
  players/sligocki
  players/tnelling
  players/vesche
Allow validate.py to inspect the PR diff
Download the PR diff from github and parse it with unidiff.  For now, just
print out what files are changed.  Future PRs can use this to do things like
have different approval requirements based on what's in the diff.

Example output, for pr #47:

```
added:
  players/TheJhyde/bonuses/initial
  players/dchudz/bonuses/initial
  players/guoruibiao/bonuses/initial
  players/jeffkaufman/bonuses/initial
  players/jmitchell/bonuses/initial
  players/pavellishin/bonuses/initial
  players/sligocki/bonuses/initial
  players/tnelling/bonuses/initial
  players/vesche/bonuses/initial
removed:
  players/TheJhyde
  players/dchudz
  players/guoruibiao
  players/jeffkaufman
  players/jmitchell
  players/pavellishin
  players/sligocki
  players/tnelling
  players/vesche
```

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

Allow transferring points with the consent of those involved
Allow a PR to go in that only transfers points.  Spoecifically, it must:

* Only have changes in the form "add a file to players/$player/bonuses/"
* Not change the total number of points.
* Have been approved by every player losing points.

Depends on #47 and #48.

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

Allow transferring points with the consent of those involved
Allow a PR to go in that only transfers points.  Spoecifically, it must:

* Only have changes in the form "add a file to players/$player/bonuses/"
* Not change the total number of points.
* Have been approved by every player losing points.

Depends on #47 and #48.
@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 9, 2019

Can't this be done with git commands?

@sligocki
Copy link

sligocki left a comment

Nice

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 9, 2019

@pavellishin "Can't this be done with git commands?"

It certainly can, but it's a bunch of work to make this work in all three of (a) Travis validate on master (b) Travis validate on PR, and (c) locally.

Is there a reason not to make a request to GitHub? It's not counted towards our API limits.

@dchudz

dchudz approved these changes Jan 12, 2019

Copy link
Contributor

dchudz left a comment

How do we think about changes to out of bounds files? Is it that you can't win on this PR? Or this PR can't be part of an overall strategy to win?

@dchudz

This comment has been minimized.

Copy link
Contributor

dchudz commented Jan 12, 2019

I think we could reorganize the requirement installs so they don't control anything that happens before validating on master. Then they wouldn't have to be out of bounds.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 12, 2019

@dchudz

I think we could reorganize the requirement installs so they don't control anything that happens before validating on master. Then they wouldn't have to be out of bounds.

That sounds like a lot of work.

Or this PR can't be part of an overall strategy to win?

More this? Specifically, that the out-of-bounds changes can't stem from trying to win?

@tnelling tnelling requested review from sligocki , guoruibiao , shaunagm , TheJhyde and vesche and removed request for sligocki Jan 14, 2019

@pavellishin pavellishin merged commit 55554bb into master Jan 17, 2019

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