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.

Sign up for GitHub

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

Jump to bottom

chore: update workflow to not close unreviewed stale PRs #3

Closed
wants to merge 2 commits into from
Closed

chore: update workflow to not close unreviewed stale PRs #3

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9200abf
chore: update workflow to not close unreviewed stale PRs
paulhcsun Dec 12, 2023
7654cde
change order of checks and fix formatting
paulhcsun Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion src/stale-prs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,10 @@ export class StalePrFinder {
console.log(' Stale since: ', stale.since.toISOString());
console.log(' Warnings: ', Object.fromEntries(warnings)); // Prints nicer

const memberReviewed = await this.memberReviewed(pull.number);

const warning = warnings.get('STALE PR');
if (!warning || warning < stale.since) {
if (!warning || warning < stale.since || !memberReviewed) {
Comment on lines +157 to +160
Copy link
Owner

Choose a reason for hiding this comment

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

Is this correct?

This is saying:

  1. If we don't have a warning yet
  2. OR the last warning we gave is from before the start of the current staleness period
  3. OR the PR hasn't been reviewed yet

THEN we should add a warning and start the timer for closing.

For fresh PRs, the 3rd condition will be automatically true, because at the start nobody will have looked at it yet.

So this would add a staleness warning immediately on every new PR!

I think what you probably want is to have memberReviewed factor into the value of stale: a PR should not be considered stale if it hasn't been reviewed yet.

Have a look at line 115, and think about how your change would interact with the conditions that are laid out there -- then make the changes over there.

Copy link
Author

Choose a reason for hiding this comment

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

Oh good catch, I was thinking this would only be run if the PR is already stale but the stale variable includes any build failures and merge conflicts as well. I'll update it with your suggestion.

// Beginning a new staleness period
action = 'warn';
} else if (warnings && this.outOfGracePeriod(warning)) {
Expand Down Expand Up @@ -340,6 +342,28 @@ export class StalePrFinder {

return ret;
}

/**
* Returns true if the PR has been reviewed or commented on by a core member.
*/
private async memberReviewed(pull_number: number): Promise<Boolean> {
const reviews = (await this.client.paginate(this.client.rest.pulls.listReviews, { ...this.repo, pull_number }));

const comments = await this.client.paginate(this.client.rest.issues.listComments, {
...this.repo,
issue_number: pull_number,
});
Copy link

Choose a reason for hiding this comment

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

Will all reviews leave comments? If so do we need to look through the reviews?

Copy link

Choose a reason for hiding this comment

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

Nit: these lines are formatted differently.

Copy link
Author

Choose a reason for hiding this comment

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

Ya I wasn't 100% sure if issues.listComments would also include reviews hence the separate check, though I don't think it'd hurt to keep the check for reviews.


// Reviews by team members
// Filtering out instances where submitted_at is empty
const memberReviews = reviews.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at);

// Comments by team members
// Filtering out instances where submitted_at is empty
const memberComments = comments.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at);

return memberComments.length > 0 || memberReviews.length > 0;
Copy link

Choose a reason for hiding this comment

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

If we are looking for either reviews or comments, can we do them one at a time. Why do we need to request all of the reviews and all of the comments all at once? Say we find a comment from a core team member, then we do not need to make a request for the reviews.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I'll change this to only check reviews if after confirming there are no core member comments.

}
}

interface ReviewState {
Expand Down
Loading