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

Add a meritocracy #377

Merged
merged 5 commits into from
May 30, 2017
Merged

Add a meritocracy #377

merged 5 commits into from
May 30, 2017

Conversation

PlasmaPower
Copy link
Contributor

@PlasmaPower PlasmaPower commented May 29, 2017

I decided to not go with a clickbait title this time 😄

Look at #361 for more details, but I tried to give the meritocracy minimal power. Democracy still decides the outcome >90% of the time. This is just to prevent against sneaky take over attempts.

Basis of how it works:

At least one positive non-author meritocracy review on the most recent commit of a PR is required for that PR to be accepted.

Changes from #361:

  • Autogenerate meritocracy list (currently top 10 contributors + top 10 voters)
  • Don't count reviews as votes (so members of the meritocracy can allow a PR without voting for it)

@mark-i-m
Copy link
Contributor

For anyone else looking through this PR, it's a lot easier to read if you append ?w=1 to the URL, which gets rid of whitespace changes (e.g. indentation changes).

@PlasmaPower
Copy link
Contributor Author

@mark-i-m yes definitely do that

hopes nobody notices my trojan horse coded in Whitespace

@phil-r
Copy link
Member

phil-r commented May 30, 2017

👍 What can possibly go wrong?

if vote and vote_owner != pr_owner:
votes[vote_owner] = vote
for vote_owner, is_current, vote in get_pr_review_votes(api, urn, pr):
if (vote > 0 and is_current and vote_owner != pr_owner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be vote_owner.lower() != pr_owner.lower()? i.e. are we sure the cases match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how it was, and it works since they both come from the GitHub API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's what I thought

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

@phil-r your comment keeps duplicating client-side:

screenshot

No idea why. It seems to be a GitHub bug. The bug survives a refresh, although the duplicated comment versions don't.

Edit: posting another comment seems to have fixed it.

@phil-r
Copy link
Member

phil-r commented May 30, 2017

@PlasmaPower I've also noticed, refresh helped, maybe I've triggered some easter egg? 🐰 🥚

@PlasmaPower
Copy link
Contributor Author

@phil-r I think it's actually a bug with reviews. It only seems to happen when the comment is next to a review. Overall, reviews are poorly implemented IMO.

@PlasmaPower
Copy link
Contributor Author

When you fix a minor mistake but it resets the voting window 😢

I guess #378 wins the merge conflict race.

@mark-i-m
Copy link
Contributor

Hey! 🎉

@mark-i-m
Copy link
Contributor

What was the fix?

@PlasmaPower
Copy link
Contributor Author

@mark-i-m In patch.py I cached gh_api.prs.get_contributors not gh_api.repos.get_contributors.

@mark-i-m
Copy link
Contributor

Btw @PlasmaPower you down voted your PR by crying in the post above...

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

@mark-i-m lol oops. 👍 I think that fixes it?

Edit: yep

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

Reminds me of when reactions to comments counted for the PR as a whole.

@PlasmaPower
Copy link
Contributor Author

Had to do another slight fix: .iteritems() was removed from Python 3. Changed to just .items()

@mark-i-m
Copy link
Contributor

@PlasmaPower Are you squashing the commits in your own fork?

@PlasmaPower
Copy link
Contributor Author

@mark-i-m Oh yeah, I probably shouldn't now that chaosbot squashes them for us. Doesn't make much of a difference though, and there's no added security - I could always amend a malicious change to a previous commit.

@mark-i-m
Copy link
Contributor

I have my 👁️ on you

@PlasmaPower
Copy link
Contributor Author

waits until @mark-i-m goes to sleep /s

@PlasmaPower
Copy link
Contributor Author

Or do you sleep with that eye open?

@mark-i-m
Copy link
Contributor

You don't know if I sleep (at the same time as you)

@PlasmaPower
Copy link
Contributor Author

I just realized I forgot to pass reverse=True to sort, so it was an opposite meritocracy... oops! Will be fixed soon, just double checking a few things.

@davidak
Copy link
Contributor

davidak commented May 30, 2017

@PlasmaPower this solution looks very good!

So at least one member of this group has to do a review seperately from voting. So people needs to know if they are in this group and that it's helpful if they review. Also that they have responsibility for the project doing it.

It would be good if you add that to the readme and we need some kind of news section to inform the community of such important changes.

@davidak
Copy link
Contributor

davidak commented May 30, 2017

@PlasmaPower If this get's merged you can mention everyone currently in the list so they know that they now have this opportunity.

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

Current meritocracy: @chaosbot, @muchzer, @pchauncey, @amoffat, @hongaar, @rhengles, @rudehn, @xyproto, @eukaryote31, @ad-m, @mark-i-m, @Smittyvb, @reddraggone9, @PlasmaPower, @phil-r

I've updated the README, pushing the changes in a second. The meritocracy will be logged every time chaosbot checks for new PRs.

(yes I know chaosbot is in the list, but hardcoding chaosbot is both unnecessary and probably a bad idea. chaosbot will also fade from the list as merge commits give way to squashes)

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

Okay, both of those changes have been pushed. Ready to merge! 🎉

Other slight updates can wait for another PR, I'll only add on important fixes.

(now to wait 9 hours because the extended voting window is broken... 😴)

@PlasmaPower
Copy link
Contributor Author

Rebased - fixed settings.py conflicts.

@PlasmaPower
Copy link
Contributor Author

PlasmaPower commented May 30, 2017

The Travis failure appears to be from master.

Edit: yep, fixed in #390

@andrewda
Copy link
Member

@PlasmaPower

@andrewda I agree, the reason I didn't implement it now is because it'd be the first dynamic part of the server.

It doesn't have to be dynamic. How I currently have it written is to update a meritocracy.json every time it's generated, just like how the voters.json works. Both approaches are a little bit gross, but we can work on refactoring that and/or using Django in another PR.

hongaar added a commit to hongaar/chaos that referenced this pull request May 30, 2017
@tarunbatra
Copy link
Contributor

This is something nice, @PlasmaPower. Though it chooses the same set of ppl, more or less, but look at the response. 😄

@PlasmaPower
Copy link
Contributor Author

@tarunbatra It's also nice that it automatically updates itself. I might adjust the config later though.

@PlasmaPower
Copy link
Contributor Author

How I currently have it written is to update a meritocracy.json every time it's generated, just like how the voters.json works.

The difference there is that voters.json needs to survive restarts, but meritocracy.json doesn't (it'd be essentially write-only). I think a dynamic server is the better solution.

@andrewda
Copy link
Member

I completely agree

@chaosbot chaosbot merged commit 3e6d99c into Chaosthebot:master May 30, 2017
@chaosbot
Copy link
Collaborator

🙆‍♀️ PR passed with a vote of 30 for and 1 against, with a weighted total of 27.5 and a threshold of 6.5.

See merge-commit 3e6d99c for more details.

@hongaar
Copy link
Member

hongaar commented May 30, 2017

well well well... another regime change by @PlasmaPower!

@andrewda
Copy link
Member

For some reason, this duplicated the object in voters.json and now it's invalid. This breaks the voters page.

{ "amoffat": 12468, "g19fanatic": 1442, ... }{ "amoffat": 12468, "g19fanatic": 1442, ... }

@PlasmaPower
Copy link
Contributor Author

That might also remove the top 10 voters part of the meritocracy. Anybody know what's causing it?

@andrewda
Copy link
Member

It's also crashing repeatedly http://chaosthebot.com:8081/chaos-stderr.log

@PlasmaPower
Copy link
Contributor Author

Oh I think that's caused by two PRs being merged at the same time. I'll get a fix ready.

PlasmaPower added a commit to PlasmaPower/chaos that referenced this pull request May 30, 2017
chaosbot added a commit that referenced this pull request May 30, 2017
commit 9f4af54
Author: Lee Bousfield <ljbousfield@gmail.com>
Date:   Tue May 30 10:45:49 2017 -0600

    Fix voters.json logic (broken by #377)
@rudehn
Copy link
Contributor

rudehn commented May 30, 2017

So what happens when somebody who is in the meritocracy reviews the PR, but falls out of the meritocracy before the PR is merged? Does the review no longer count?

@andrewda
Copy link
Member

I believe that the next time the PR gets polled, the review will no longer count.

@rudehn
Copy link
Contributor

rudehn commented May 30, 2017

Is that working as intended, or a bug?

@PlasmaPower
Copy link
Contributor Author

That's how it works, and there's really no way to fix it unless we get the database up and running (even then it'd be a bit of a pain).

@mdcfe
Copy link
Contributor

mdcfe commented May 30, 2017

@PlasmaPower This is supposed to be chaos - who knows?

chaosbot pushed a commit that referenced this pull request May 31, 2017
#392: Update README in anticipation of #377

Description:


:ok_woman: PR passed with a vote of 20 for and 0 against, a weighted total of 19.5 and a threshold of 6.5, and a current meritocracy review.

Vote record:
@Ayyko: 1
@Leigende: 1
@MUCHZER: 1
@Mursaat: 1
@Smittyvb: 1
@SylvainThrd: 1
@andrewda: 1
@cthulhuely: 1
@davidak: 1
@eukaryote31: 1
@flibustier: 1
@hongaar: 1
@ike709: 1
@md678685: 1
@mikesmiffy128: 1
@phil-r: 1
@reddraggone9: 1
@rhengles: 1
@rudehn: 1
@viktorsec: 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants