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

switch to named bonuses #47

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 9, 2019

Right now you can easily have merge conflicts if two PRs both try to give the same user points.

Use the directory trick to tell git these are independent: give each player a directory of named bonuses, and add them up to get points.

I've added this adding-up to #33

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

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.

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

Add get_user_points
Add a function to pull points out of the points files, and print them.  Still
doesn't make points do anything.  Pulled out of #33 to make things easier to
review.

During review of #33 people noted that you might be able to make this function
fail with an Exception saying that you win, which might not be caught earlier
because get_user_points would only be called in determine_if_winner, which
isn't called on PR validation.  One way to handle this would be to rewrite the
code defensively, catching every potential issue (something not being a
directory, contents of the file not being an int, etc), but this PR takes a
much simpler (and easier to trust approach) of just additionally running
get_user_points on PR validation.  This way, if there's something wrong with
the environment the PR will fail in validation and not be mergeable.

Depends on #47, compatible with #33

@jeffkaufman jeffkaufman referenced this pull request Jan 9, 2019

Open

Add get_user_points #50

@TheJhyde

This comment has been minimized.

Copy link
Collaborator

TheJhyde commented Jan 10, 2019

This seems... complicated. Are merge conflicts really so bad? I'm not going to reject this but I won't approve.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 10, 2019

@TheJhyde

This seems... complicated. Are merge conflicts really so bad? I'm not going to reject this but I won't approve.

Merge conflicts mean that every time something gets merged to master all outstanding PRs need to start over with getting approved. So if we want points to be easy to use then I think this is a lot better.

@tnelling tnelling added the Points label Jan 11, 2019

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

named-bonuses: alternative implementation
This is an alternative to #47, where the point value is in the filename instead of the file body.  Suggested in the comments on #50, the idea is this makes it easier to parse diffs that work with points.

I also added two example files to my directory, one showing point gain and one showing point loss, so you can see what negative numbers look like.

I don't have strong feelings between this and #47, and would be happy with either.
@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 17, 2019

@jeffkaufman whoops, conflicts now.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 17, 2019

If that doesn't prove that we should go with #57, I don't know what would :P

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

@tnelling

If that doesn't prove that we should go with #57, I don't know what would

#57 also has conflicts.

This is something that will be fixed by moving away to named bonuses, however we handle them.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 17, 2019

Oh right never mind.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

Resolved merge conflicts. Not currently planning on doing the same to #57 since I think #57 is silly, but if someone asks me to I will.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

also hold off until #33 is in, because #33 will have conflicts as well

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