-
Notifications
You must be signed in to change notification settings - Fork 30
Add to mergeable queue on PR push, open and reopen #275
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
Conversation
src/bors/handlers/pr_events.rs
Outdated
db.set_pr_status( | ||
let pr = &payload.pull_request; | ||
let pr_number = pr.number; | ||
db.get_or_create_pull_request( |
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 should really be named db.upsert_pull_request(...)
rather than db.get_or_create_pull_request(...)
because it updates the PR's base_branch
, mergeable_state
and status
.
Will update it if you agree.
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.
Good point, a more accurate name is get_or_update
, which is essentially just upsert
.
src/bors/handlers/pr_events.rs
Outdated
.lock() | ||
.get_pr_mut(default_pr_number()) | ||
.mergeable_state = OctocrabMergeableState::Unknown; | ||
tester |
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.
We know that a push always sets the unknown state, so we could add the logic of setting Unknown
directly to push_to_pr
, to make sure that we don't forget it, and so that we don't have to do it in every test that checks the mergeability 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.
Thanks!
Needs a reformat :) |
0a17b58
to
3a4348b
Compare
3a4348b
to
c0ce5fc
Compare
Related #218
This PR adds to the mergeable queue on: