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 transferring points with the consent of those involved #49

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 9, 2019

Allow a PR to go in that only transfers points. Specifically, 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.

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.
@sligocki
Copy link
Collaborator

sligocki left a comment

I like this idea, but this will allow people to manufacture points very easily because they will bet a point for submitting a bonus transfer with very small approval threshold.

One way around this is to exclude these PRs from gaining points.

I think my proposal is in the spirit of the original points for PRs rule as well. We give points for PRs because we want to encourage PRs, but we don't necessarily want to encourage arbitrary exchanges of points.

@@ -128,6 +180,10 @@ def determine_if_mergeable():
else:
rejections.append(user)

if mergeable_as_points_transfer(diff, approvals, rejections):

This comment has been minimized.

@pavellishin

pavellishin Jan 9, 2019

Collaborator

Where's diff defined?

This comment has been minimized.

@jeffkaufman
@pavellishin
Copy link
Collaborator

pavellishin left a comment

This code is getting into the territory where some tests would be useful.

Is there a simpler way to do this? There's a lot of explicit indices and string comparisons going on. Can we do something conceptually simpler, like checking out master and summing up all the users' points, followed by checking out the current PR and summing all the users' points, and doing arithmetic based on that?

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 9, 2019

this will allow people to manufacture points very easily

I'm fine with that, because the points don't matter anyway :)

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 9, 2019

I've also noticed a trend - we're also getting a lot of PRs that depend on other PRs. :/ I think some people cough Jeff cough have more time to contribute to this than others :P

I think in the future once we've ironed out some kinks, we'll want to run parallel games at different speeds.

Or maybe once this game is over, we should try to do more of a speed-round - I understand concerns about people who can only check in once or twice a day, but it would be interesting to see how this game plays out if it's quick, and if most people end up not actually reviewing PRs that get merged in until they're already in master.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@sligocki

this will allow people to manufacture points very easily because they will bet a point for submitting a bonus transfer with very small approval threshold.

That's a very good point, and one I wasn't considering. I'll rewrite it to fix that once #33, #47, and #48 are in.

@pavellishin pavellishin added the blocked label Jan 9, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@pavellishin

I think some people cough Jeff cough have more time to contribute to this than others

Not really. I should be spending much less time on this. Hopefully I'll either lose interest somewhat, or have better discipline, but other people taking a long time to review things is really a good thing for me, all things considered.

@sligocki

This comment has been minimized.

Copy link
Collaborator

sligocki commented Jan 9, 2019

this will allow people to manufacture points very easily

I'm fine with that, because the points don't matter anyway :)

... presumably they will matter for something otherwise why are we putting so much effort in to quantify them :P

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@pavellishin

Is there a simpler way to do this? There's a lot of explicit indices and string comparisons going on. Can we do something conceptually simpler, like checking out master and summing up all the users' points, followed by checking out the current PR and summing all the users' points, and doing arithmetic based on that?

That's possible, and I considered it, but I think we're going to want other parts of validate.py that look at the contents of a PR and make a decision about what conditions to apply. I think that's much easier if we start building things that look at the diff directly.

The explicit indices and string manipulation are me trying to be paranoid about what counts. I'm pretty sure the following check would return the same results under all circumstances:

if (len(path_components) != 4 or
    len(diff_lines) != 8 or
    path_components[0] != 'players' or
    path_components[2] != 'bonuses' or
    diff_lines[7] != ''):
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

@pavellishin but I can still make this much more readable, and I'll do that when I come back to it.

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 10, 2019

Thanks, I would appreciate it!

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