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

Require all PRs to wait 1 day after creation before merging, unless they're unanimously approved #44

Merged
merged 7 commits into from Jan 23, 2019

Conversation

@tnelling
Copy link
Collaborator

tnelling commented Jan 9, 2019

Note that this is from PR creation, not from any subsequent activity.

There's a lot of activity in this game taking place during the day while I'm at work, while I'm generally only able to review said activity after I get home. I'm concerned that with the 2/3 majority rule in effect, I'll have to start rejecting things at work so that I have time to review later, which I don't want to have to do for both in-game and out-of-game reasons.

This obviously should be updated to reflect the refactoring in #37 once that's merged.

@tnelling tnelling changed the title Require all PRs to wait 1 day before merging Require all PRs to wait 1 day after creation before merging Jan 9, 2019

@jeffkaufman
Copy link
Owner

jeffkaufman left a comment

I don't think this PR remotely works. Did you test it?

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

No, I know it's not ready. I wanted to get Travis set up to run it via GitHub rather than locally (you did say part of the goal was to learn CI!). But I figured there would be objections to the idea anyway so I wanted to get the idea out there.

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

Also I just realized that I didn't actually do the time conversion, so that's certainly one problem.

@tnelling tnelling added the Don't Merge label Jan 9, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

Added Don't Merge to reflect this

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 9, 2019

If you have something that's not expected to work yet, best to put that in the PR description ;)

@tnelling tnelling self-assigned this Jan 9, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

I have tested locally with my addition of actually doing the time conversion and it seems to work. But I'd still rather see #37 go in first anyway.

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

That said, I'd still like to figure out how to get my fork running Travis on GitHub.

@tnelling tnelling changed the title Require all PRs to wait 1 day after creation before merging Require all PRs to wait 1 day after creation before merging, unless they're unanimously approved Jan 9, 2019

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 9, 2019

Updated to allow merging within 24 hours if the PR has unanimous approval

@pavellishin
Copy link
Collaborator

pavellishin left a comment

This will probably need to be fixed after #37, but I think I'm fine with this in principle.

If it ends up slowing the game too much, we can argue about it via PRs :)

@tnelling tnelling added blocked and removed Don't Merge labels Jan 10, 2019

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 10, 2019

You know what, based on the speed of this game, I don't think we need this at all. But I'm going to leave my approval, because I think it also doesn't hurt aynthing.

@tnelling

This comment has been minimized.

Copy link
Collaborator Author

tnelling commented Jan 11, 2019

I think the speed of the game is currently being limited quite a bit by #41. That's slowing down the rate most PRs can go in, but most notably #37, which is a blocker for several other PRs. Once that's all cleared up, I could see things speeding up quite a bit.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 11, 2019

@tnelling #37 is in now if you want to pick this back up, and I'll approve once it's updated to use those utility functions.

(Unrelated: can you look at #33, please?)

@tnelling tnelling removed the blocked label Jan 11, 2019

@tnelling tnelling requested review from sligocki , dchudz and guoruibiao Jan 14, 2019

@tnelling tnelling requested review from shaunagm , TheJhyde and vesche Jan 14, 2019

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 14, 2019

@tnelling this needs merge conflicts resolved

validate.py Outdated
@@ -4,6 +4,7 @@
import requests
import subprocess
import time
import datetime

This comment has been minimized.

@jeffkaufman

jeffkaufman Jan 14, 2019

Owner

not used, though not important enough to wipe approvals over

This comment has been minimized.

@tnelling

tnelling Jan 14, 2019

Author Collaborator

Oops, that was left in from the pre-time-refactor version.

@jeffkaufman

This comment has been minimized.

Copy link
Owner

jeffkaufman commented Jan 17, 2019

@tnelling merge conflicts!

@vesche

vesche approved these changes Jan 18, 2019

@tnelling tnelling merged commit f22fb48 into jeffkaufman:master Jan 23, 2019

@jeffkaufman jeffkaufman referenced this pull request Jan 23, 2019

Closed

Steadily decrease veto power as PR ages #35

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment