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
Fix ping stale reviewers on subsequent reviews #293
base: master
Are you sure you want to change the base?
Conversation
4346ceb
to
f7c6bb3
Compare
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 confused here. The current code in pr-info
chooses the most recent review for each reviewer, and then the code that your changing here takes the earliest of these which I think makes sense (but not sure). With this change, it would potentially make more nagging notifications when there are new reviews that become stale--??
One thing that is definitely missing (and would help clarify the above) is if you have a specific PR in mind where this change can be demonstrated (and used as a new test).
@@ -59,7 +57,7 @@ ${names.map(n => `${n}`).join(" ")} | |||
}); | |||
|
|||
export const PingStaleReviewer = (reviewedAbbrOid: string, reviewers: string[]) => ({ | |||
tag: `stale-ping-${sha256(reviewers.join("-")).substr(0, 6)}-${reviewedAbbrOid}`, | |||
tag: `stale-ping-${reviewedAbbrOid}`, |
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.
Why are you dropping the reviewers' hash too?
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.
A change in reviewers
implies a change in reviewedAbbrOid
, which the tag already contains, because the only way to change the stale reviewers is for a new review to become stale.
I added 49699 as a test (I had to edit it slightly because the CI isn't passing today). I ran
The most recent reviews are dt-mergebot/src/_tests/fixtures/49699/derived.json Lines 126 to 137 in b8021e7
If we use the I confirmed that switching from |
1d39a68
to
9e931d1
Compare
#79 put
mostRecentReview.reviewedAbbrOid
in the ping stale reviewers comment tag however.sort((l, r) => l.date.localeCompare(r.date))[0]
in fact returns the least-recent review https://github.com/DefinitelyTyped/dt-mergebot/pull/79/files#diff-0dfef229af227cfaffa9c063923ac5b13c53e14422a6147d11bb95c28d98f37eR204Since there are never any new least-recent reviews this makes that part of the tag impotent: subsequent reviews get the same tag.
This PR replaces
min()
withmax()
to get the behavior that presumably was intended.