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

approval-threshold: lower it daily after three days with no commits #23

Merged
merged 5 commits into from Jan 6, 2019

Conversation

Projects
None yet
6 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 5, 2019

Right now, if someone disappears then we'll have a draw; see #19. This change is a straight-forward way around this: once three days have passed without a commit, then for every additional day we require one fewer approver.

I'm open to other functions, but I'd like to get something in soon because we're adding a lot of players.

(I think this is probably not what we'll want long term.)

jeffkaufman added some commits Jan 5, 2019

approval-threshold: lower it daily after three days with no commits
Right now, if someone disappears then we'll have a draw; see #19.  This change is a straight-forward way around this: once three days have passed without a commit, then for every additional day we require one fewer approver.

I'm open to other functions, but I'd like to get something in soon because we're adding a lot of players.

(I think this is probably not what we'll want long term.)
@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 5, 2019

Doesn't this mean that even if everyone rejects the PR, eventually it gets merged in? Should we account for rejection, and should we have a floor for number of approvers?

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 6, 2019

@tnelling

Doesn't this mean that even if everyone rejects the PR, eventually it gets merged in?

Only if nothing else gets merged in the meantime. Basically, if gameplay gets hung, then the threshold starts dropping.

@jeffkaufman jeffkaufman referenced this pull request Jan 6, 2019

Closed

Relax rules for quitting #25

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 6, 2019

Doesn't this mean that even if everyone rejects the PR, eventually it gets merged in?

Only if nothing else gets merged in the meantime. Basically, if gameplay gets hung, then the threshold starts dropping.

Fair enough. But nothing merging isn't the same thing as gameplay being hung, imo. If people are working on PRs, that's not accounted for, because this only checks against master.

Still, I'd probably be fine with letting the threshold drop after 3 days (though preferably longer I think) if people simply aren't approving or rejecting. In other words, turning abstention into approval (abstention being what we're trying to address). What I don't like is that this eventually turns rejections into approvals.

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

players: switch to a directory
Currently, when new players are added that makes merge conflicts, and requires
a lot of re-approvals.  An easy way to avoid this is to use a directory instead
of a file.

Also add everyone who has asked to be added:
 * Closes #11
 * Closes #12
 * Closes #15
 * Closes #20
 * Closes #22

We should probably merge #23 first, so we don't get stuck if we lose someone
(#19).
@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 6, 2019

@tnelling

I'd probably be fine with letting the threshold drop after 3 days (though preferably longer I think) if people simply aren't approving or rejecting

I'm fine with that, except that (a) it's much harder to code and (b) I really want to get something in soon before we add a lot more players. We don't have to stick with this.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 6, 2019

I don't see a problem with allowing rejections to be an automatic failure for now, which wouldn't be hard to add. We may not want to require unanimous approval, but that's a separate issue from dealing with abstentions, in my opinion.

Show resolved Hide resolved validate.py Outdated
@jmitchell

This comment has been minimized.

Copy link
Collaborator

jmitchell commented Jan 6, 2019

I would also like to see explicit abstention rejection carry more weight than no feedback. Otherwise, I support the motivation behind this PR.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 6, 2019

@tnelling updated the PR: now if anyone rejects merging isn't allowed

@jmitchell I'm not sure what you mean by "explicit abstention"?

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 6, 2019

@dchudz @TheJhyde can you review?

This was referenced Jan 6, 2019

@dchudz

dchudz approved these changes Jan 6, 2019

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 6, 2019

I would also like to see explicit abstention carry more weight than no feedback.

@jmitchell I'm not sure what you mean by "explicit abstention"?

Also, what side would it weigh in on and what would it mean?

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 6, 2019

Nevermind, I think I get it. It sounds like the countdown should just lower the total number of people whose explicit participation is required.

@TheJhyde TheJhyde merged commit 47a8ae8 into master Jan 6, 2019

@jmitchell

This comment has been minimized.

Copy link
Collaborator

jmitchell commented Jan 6, 2019

Apparently, I hadn't had enough ☕️ the other day. I meant to contrast explicit rejection against implicit abstention.

I was voicing agreement with @tnelling's comment:

I don't see a problem with allowing rejections to be an automatic failure for now, which wouldn't be hard to add.

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