Skip to content

Commit

Permalink
Use draft pull requests instead of the 'Work In Progress' and 'Needs …
Browse files Browse the repository at this point in the history
…Review' labels. (#56168)
  • Loading branch information
Cyberboss authored Jan 16, 2021
1 parent e015c32 commit 96d5f6a
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 77 deletions.
4 changes: 3 additions & 1 deletion .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -572,10 +572,12 @@ There is no strict process when it comes to merging pull requests. Pull requests

* Please explain why you are submitting the pull request, and how you think your change will be beneficial to the game. Failure to do so will be grounds for rejecting the PR.

* If your pull request is not finished make sure it is at least testable in a live environment. Pull requests that do not at least meet this requirement will be closed. You may request a maintainer reopen the pull request when you're ready, or make a new one.
* If your pull request is not finished, you may open it as a draft for potential review. If you open it as a full-fledged PR make sure it is at least testable in a live environment. Pull requests that do not at least meet this requirement will be closed. You may request a maintainer reopen the pull request when you're ready, or make a new one.

* While we have no issue helping contributors (and especially new contributors) bring reasonably sized contributions up to standards via the pull request review process, larger contributions are expected to pass a higher bar of completeness and code quality *before* you open a pull request. Maintainers may close such pull requests that are deemed to be substantially flawed. You should take some time to discuss with maintainers or other contributors on how to improve the changes.

* After leaving reviews on an open pull request, maintainers may convert it to a draft. Once you have addressed all their comments to the best of your ability, feel free to mark the pull as `Ready for Review` again.

## Porting features/sprites/sounds/tools from other codebases

If you are porting features/tools from other codebases, you must give them credit where it's due. Typically, crediting them in your pull request and the changelog is the recommended way of doing it. Take note of what license they use though, porting stuff from AGPLv3 and GPLv3 codebases are allowed.
Expand Down
78 changes: 2 additions & 76 deletions tools/WebhookProcessor/github_webhook_processor.php
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ function tag_pr($payload, $opened) {
$tags[] = $tag;

check_tag_and_replace($payload, '[dnm]', 'Do Not Merge', $tags);
if(!check_tag_and_replace($payload, '[wip]', 'Work In Progress', $tags) && check_tag_and_replace($payload, '[ready]', 'Work In Progress', $remove))
$tags[] = 'Needs Review';

return array($tags, $remove);
}
Expand All @@ -287,81 +285,11 @@ function get_reviews($payload){
return json_decode(github_apisend($payload['pull_request']['url'] . '/reviews'), true);
}

function check_ready_for_review($payload, $labels = null, $remove = array()){
$r4rlabel = 'Needs Review';
$labels_which_should_not_be_ready = array('Do Not Merge', 'Work In Progress', 'Merge Conflict');
$has_label_already = false;
$should_not_have_label = false;
if($labels == null)
$labels = get_labels($payload);
$returned = array($labels, $remove);
//if the label is already there we may need to remove it
foreach($labels as $L){
if(in_array($L, $labels_which_should_not_be_ready))
$should_not_have_label = true;
if($L == $r4rlabel)
$has_label_already = true;
}

if($has_label_already && $should_not_have_label){
$remove[] = $r4rlabel;
return $returned;
}

//find all reviews to see if changes were requested at some point
$reviews = get_reviews($payload);

$reviews_ids_with_changes_requested = array();
$dismissed_an_approved_review = false;

foreach($reviews as $R)
if(is_maintainer($payload, $R['user']['login'])){
$lower_state = strtolower($R['state']);
if($lower_state == 'changes_requested')
$reviews_ids_with_changes_requested[] = $R['id'];
else if ($lower_state == 'approved'){
dismiss_review($payload, $R['id'], 'Out of date review');
$dismissed_an_approved_review = true;
}
}

if(!$dismissed_an_approved_review && count($reviews_ids_with_changes_requested) == 0){
if($has_label_already)
$remove[] = $r4rlabel;
return $returned; //no need to be here
}

if(count($reviews_ids_with_changes_requested) > 0){
//now get the review comments for the offending reviews

$review_comments = json_decode(github_apisend($payload['pull_request']['review_comments_url']), true);

foreach($review_comments as $C){
//make sure they are part of an offending review
if(!in_array($C['pull_request_review_id'], $reviews_ids_with_changes_requested))
continue;

//review comments which are outdated have a null position
if($C['position'] !== null){
if($has_label_already)
$remove[] = $r4rlabel;
return $returned; //no need to tag
}
}
}

//finally, add it if necessary
if(!$has_label_already){
$labels[] = $r4rlabel;
}
return $returned;
}

function check_dismiss_changelog_review($payload){
global $require_changelog;
global $require_changelogs;
global $no_changelog;

if(!$require_changelog)
if(!$require_changelogs)
return;

if(!$no_changelog)
Expand Down Expand Up @@ -406,8 +334,6 @@ function handle_pr($payload) {
check_dismiss_changelog_review($payload);
case 'synchronize':
list($labels, $remove) = tag_pr($payload, false);
if($payload['action'] == 'synchronize')
list($labels, $remove) = check_ready_for_review($payload, $labels, $remove);
set_labels($payload, $labels, $remove);
return;
case 'reopened':
Expand Down

0 comments on commit 96d5f6a

Please sign in to comment.