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.

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

Create 0.17-allow-new-players #226

Merged
merged 11 commits into from Feb 15, 2019

Conversation

Projects
None yet
4 participants
@tnelling
Copy link
Collaborator

tnelling commented Feb 10, 2019

Looking for design review on this in two respects:

  1. How can we prevent dummy account creation and subsequent point transfers, as discussed in the first game? One thought I had was to start new players at 0 (though effectively 1 if they create the PR to join), making this a way of getting into the game faster at the cost of some points. Another idea I had was to introduce a constraint on transfers for players who have no points for anything except their initial bonus and their join PR merge point.

  2. There's a bunch of code copied from 0.3-allow-points-transfer.py. I'd like feedback on the best way to deal with that. Should we have a diff class?

@pavellishin

This comment has been minimized.

Copy link
Contributor

pavellishin commented Feb 11, 2019

What if we allow players to join without review if they're willing to take zero points as their starting value? (Or maybe even a negative point value.)

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Feb 11, 2019

What if we allow players to join without review if they're willing to take zero points as their starting value? (Or maybe even a negative point value.)

That's exactly what I said in my initial comment :P

Well, minus the negative option. I guess we could go that route, though I'm not sure it wouldn't break the points transfer stuff as currently written.

@pavellishin

This comment has been minimized.

Copy link
Contributor

pavellishin commented Feb 11, 2019

That's exactly what I said in my initial comment :P

It's not easy keeping up my reputation as an illiterate, you know! It takes hard work :P

And hm, I forgot we forbade negative point values in any given PR. That would probably fuck up the winning-by-points calculation, although maybe we could just remove players with negative points from consideration altogether, and allow negative points only on new player creation (or whenever point transfers didn't result in someone's score going further into the red.)

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

factor out get_new_bonuses_or_raise
In #226 @tnelling is trying to add something to make it easier to add new players, which adds a second use case for "parse a diff, figure out if all changes are additions of bonus files, and if so parse them".  I've now factored this out into por.get_new_bonuses_or_raise() and updated 0.3-allow-points-transfer.py to use it
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Feb 11, 2019

Approach LGTM, I'll approve once rebased off #228

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Feb 12, 2019

@tnelling #228 is merged; this is ready to pick up again

tnelling added some commits Feb 12, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Feb 12, 2019

Updated. I decided to set the max bonus value to 0 for now until we find a way to address the dummy account question. Personally, I'd be okay with ruling dummy accounts out-of-bounds. Any given player should only participate via a single account. But I do sympathize with the desire to minimize what's considered out-of-bounds.

If we're willing to go the out-of-bounds route, I'm happy to update this PR to reflect as much in the readme, and change the max starting bonus accordingly.

@tnelling tnelling added reviewme and removed do-not-merge labels Feb 12, 2019

elif len(bonuses) < 0:
raise Exception('Empty diff?')

(points_user, points_name, points_change) = bonuses[0]

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 12, 2019

Owner

What about replacing:

  if len(bonuses) > 1:
    raise Exception('Only one new player can be added in a PR')
  elif len(bonuses) < 0:
     raise Exception('Empty diff?')

  (points_user, points_name, points_change) = bonuses[0]

with just:

  (points_user, points_name, points_change), = bonuses

This comment has been minimized.

@tnelling

tnelling Feb 12, 2019

Author Collaborator

Are you saying it will throw if there's not exactly one element? Even if so, it's less clear what happened.

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 12, 2019

Owner

Yes, this throws if it can't destructure exactly as planned. But ok if you don't like it.

This comment has been minimized.

@tnelling

tnelling Feb 12, 2019

Author Collaborator

I could do a try statement, not sure if that's better. I just like the specificity of the failure messages.

@jeffkaufman jeffkaufman requested a review from jburger424 Feb 12, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Feb 12, 2019

@pavellishin @jburger424 needs review

jeffkaufman and others added some commits Feb 13, 2019

@jeffkaufman
Copy link
Owner

jeffkaufman left a comment

This needs to be called rules/0.17-allow-new-players.py

Made requested changes before the review was actually submitted

@jeffkaufman jeffkaufman merged commit e6fcf1e into master Feb 15, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
(points_user, points_name, points_change) = bonuses[0]

if points_user in util.users():
raise Exception('Cannot create an existing user')

This comment has been minimized.

@jeffkaufman

jeffkaufman Feb 15, 2019

Owner

This isn't going to work: when this runs on the master branch it will succeed (because points_user will be a new user), but when it runs on the PR branch it will fail (because points_user will not be a new user on the PR branch). You don't need this check, though, since you're already requiring that this PR add a file called initial, which will already exist for any current user.

See #243 for an example of this rule failing an example PR to add a user: https://travis-ci.com/jeffkaufman/nomic/builds/101126028

...
Running rule rules/0.17-allow-new-players.py
PASS: new-players

...

Running rule rules/0.17-allow-new-players.py
Traceback (most recent call last):
  File "validate.py", line 84, in determine_if_mergeable
    if fn(pr_copy):
  File "rules/0.17-allow-new-players.py", line 17, in should_allow
    raise Exception('Cannot create an existing user')
Exception: Cannot create an existing user
  rules/0.17-allow-new-players.py: Cannot create an existing user
...

The PASS is on master, then the FAIL is on the branch.

@jeffkaufman jeffkaufman referenced this pull request Feb 15, 2019

Closed

ban sockpuppets? #245

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.