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.

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 empty owners-to-ping in staleness comments #439

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Apr 25, 2022

Motivated by a trivial cosmetic glitch (spurious /) in the Unmerged:nearly comment: Screenshot from 2022-04-25 11-31-23

It was introduced in a change that instead targeted this Unreviewed: comment.

The fix proposed here is, don't filter existing approvals out of Unreviewed: (and Unmerged:) pings. Instead, don't apply the unreviewed label/timeline to PRs that have reviews and are already in the maintainer queue.

The unreviewed timeline has two functions, I think:

  1. Bumping unreviewed PRs to the maintainer queue.
  2. Soliciting reviews (for PRs in the maintainer queue and ones that aren't).

It doesn't have a function if a PR in the maintainer queue has reviews?

  • Doesn't change the project column.
  • Shouldn't re-ping anyone.

The code already treats "unreviewed" as completely unreviewed vs. unreviewed by maintainers, I think:

This change:

  • Fixes the cosmetic glitch (for Unmerged: and Unreviewed: comments).
  • Retains Avoid redundant pings #367.
  • Makes the unreviewed label/timeline more consistent with its usage (completely unreviewed).

@@ -339,7 +339,7 @@ export function process(prInfo: BotResult,
// Has it: got no DT tests but is approved by DT modules and basically blocked by the DT maintainers - and it has been over 3 days?
// Send a message reminding them that they can un-block themselves by adding tests.
if (!info.hasTests && !info.hasMultiplePackages && info.approvedBy.includes("owner") && !info.editsInfra
&& info.approverKind === "maintainer" && (info.staleness?.days ?? 0) > 3) {
&& info.approverKind === "maintainer" && dayjs(info.now).diff(info.lastPushDate, "days") > 3) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't apply the unreviewed timeline to these PRs anymore (ones in the maintainer queue with reviews). This replacement is the same as if we did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant