-
Notifications
You must be signed in to change notification settings - Fork 210
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
[WIP] brainstorming for /vote fast + prototype [WILL NOT BE UN-WIP-ED] #493
Conversation
Downvoting because the code doesn't do anything (aside from create space for some potential future code) and the PR isn't clear enough. If you make the code do something and the PR says what the code does, then maybe I'll upvote.
I don't even know how the feature is supposed to work, so how a I going to know what functions might be needed? I'd like to evaluate your judgement and design decisions, but I don't know how the feature you want to build works...and you didn't update the code with the feature...so this is all a bit of a mystery. |
@anythingbot The proposal for the feature is... wait, he already linked it: This is marked WIP - it's clearly not finished yet, and mark even said that this is only the beginning of the work for it. |
@mark-i-m you will need to change the |
Wait, does chaosbot avoid automatically merging [WIP] PRs? I didn't realize that was something it did. |
Yep. If it has |
DB is in, this can be finished |
@rudehn Can you confirm that I am not messing up the database stuff? |
Walk me through what you are trying to do. What does |
|
cron/poll_pull_requests.py
Outdated
try: | ||
issue = Issue.get(issue_id=pr["id"]) | ||
if issue.expedited: # expedite | ||
voting_window /= 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code needs to be moved out of the if statement for the extended voting window. Currently this just halves the extended voting window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also a bunch of flake8 warnings, and some of them are critical (e.g. you forgot to import json
in cron/poll_read_issue_comments.py
).
cron/poll_read_issue_comments.py
Outdated
with open('server/meritocracy.json', 'r') as mfp: | ||
fs = fp.read() | ||
if fs: | ||
meritocracy = json.loads(fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit flaky. How about defining a function get_meritocracy
with the current code for generating the meritocracy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server/meritocracy.json
is temporary - it shouldn't ideally be read from because it's only there to fill the gap before the database is ready and we can use that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@md678685 What should I do instead? Would it be ok to define get_meritocracy
as @PlasmaPower said and simply re-implement it when the db is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would need re-implemented when the DB is ready. Sure, the DB could be used, but there's no point in that since this code and the main loop (PRs) run in the same process (so they share the same memoization cache).
cron/poll_pull_requests.py
Outdated
voting_window /= 2 | ||
|
||
except Issue.DoesNotExist: | ||
pass # not expedited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, add an except: log.exception("Failed to get expedited status")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PlasmaPower This is not necessarily a failure, though... It just means nobody has posted /vote fast
on this PR. In fact, we expect that to be the common case, so these would flood the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure you understand. I'm saying keep the current error handler, but add another general one in case a different exception happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad... fixed
@mark-i-m Don't you also want to require additional meritocracy reviews? Also, should the author of the PR be able to |
@mark-i-m this can be done with a mysql database, but this isn't necessary; it can also be done with a github label. That way the bot can check for labels on the PR to see if the "expedited" label is present. If so, the bot cuts the voting window in half. |
@dbpokorny That's a great idea! I'm not sure it'll be necessary though, since we do have a DB and an implementation using it. |
Adding the label does sound like a good way of visually differentiating fast PRs on the PR list, though... |
@PlasmaPower My whole take on expedited PRs is that it effectively turns the chaosbot into a bicameral legislature; so I think either method is fine. Part of the issue here is that using a mysql database doesn't reveal the status of the PR using the github web interface; you'll still have to add the github label in order to signal the status of the PR to github users. So, the thinking goes, if github is going to remember the status, then there isn't any need (on this project) for additional mysql configuration. I'm under the impression that the only data this database would store is expedited status for each PR. Is that correct? |
@dbpokorny That is a good point in favor of a label. I guess we could add a label, but pull the info from the DB. And yes, I think you're correct about the DB model. |
@PlasmaPower pull what info from the database? I don't understand what that is |
cron/poll_read_issue_comments.py
Outdated
|
||
# db insert | ||
issue, _ = Issue.get_or_create(issue_id=issue_id, | ||
number=number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone know what the difference between these is? Can I drop one of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend using the defaults
param here for anything not in the primary key. We were having issues using this syntax before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better?
Ok, I think I have code representing roughly what has been discussed above. I really doubt it runs in its current state. Mostly, I would appreciate feedback from reviews about what should be changed. After that, I will fix up broken things and submit a new PR. Think of this PR as pseudocode for the real one... |
Will be back tomorrow 💤 |
Sorry, I didn't see this earlier. I don't think so. If it is something that should be |
First issues to fix should be Travis. There are some undefined names and such. |
cron/poll_read_issue_comments.py
Outdated
|
||
# db insert | ||
issue, _ = Issue.get_or_create(issue_id=issue_id, | ||
number=number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend using the defaults
param here for anything not in the primary key. We were having issues using this syntax before
user, _ = User.get_or_create(user_id=cmd_obj["user"]["id"], | ||
defaults={"login": cmd_obj["user"]["login"]}) | ||
|
||
issue = insert_or_update_issue(api, cmd_obj["issue_id"], cmd_obj["number"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where are you getting cmd_obj["number"]
from? I don't think this exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it ought to be just returned from github, right: https://developer.github.com/v3/issues/#list-issues-for-a-repository?
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Read comments.py / get_all_issue_comments. It doesn't return the entire structure. You might need to change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch. Fixed
cron/poll_read_issue_comments.py
Outdated
return | ||
|
||
# The post should not have been updated | ||
if updated_at != created_at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe leave some kind of response indicating why the command didn't run? Makes debugging easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yes, I see your point... but would this be prone to inducing spam?
@@ -28,6 +28,11 @@ class Comment(BaseModel): | |||
|
|||
class Issue(BaseModel): | |||
issue_id = pw.IntegerField(primary_key=True) | |||
number = pw.IntegerField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR number as oppose to PR id? The thing is when github returns a PR, it returns a value called "id" which is not the PR number we see on github, and it returns a "number" which is the number we see on github. TBH, I am really confused about which to use where. I think we want to use issue/PR numbers, rather than ids, but the db models appear to use the ids. Any help/auditing on this point would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ids are used when making api calls. I think that's why we store them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... interesting. Most of the API I saw for issues and PRs take the issue/PR number, though?
@@ -145,6 +178,48 @@ def handle_vote_command(api, command, issue_id, comment_id, votes): | |||
gh.issues.close_issue(api, settings.URN, issue_id) | |||
elif sub_command == "reopen": | |||
gh.issues.open_issue(api, settings.URN, issue_id) | |||
elif sub_command == "fast": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe pull this out into it's own function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cron/poll_read_issue_comments.py
Outdated
@@ -135,7 +157,63 @@ def get_command_votes(api, urn, comment_id): | |||
return votes | |||
|
|||
|
|||
def handle_vote_command(api, command, issue_id, comment_id, votes): | |||
def get_meritocracy(): | |||
# FIXME: update when this is done by the db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the meritocracy will ever be stored in the DB. Instead, put this code in the function.
This code should also be changed to use the |
I would like to close this PR to clean up my dashboard. Does anyone have objections? |
As with the other issue, no objection here. |
Start work for /vote fast
My proposal is here in #401 .
This PR is not intended to implement any feature, but to implement some needed functions/methods/utilities that might be needed, and to just plan in general.The actual feature will probably wait for @rudehn's work on database stuff.Thoughts and comments appreciated...
EDIT: I added some code to implement this feature now that @rudehn's work is in... I will probably not un-WIP this. Rather, when it looks good, I will create a new PR, so as not to take advantage of those who already voted.