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

Allow owners to unsubscribe from some PRs #461

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

Allow owners to unsubscribe from some PRs #461

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c27208
Allow owners to unsubscribe from some PRs
fregante Apr 12, 2023
0475402
Restore first ping
fregante Jan 20, 2024
140ec30
Update snapshots
fregante Jan 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const SuggestTesting = deletedWhenNotPresent("suggest-testing", tag =>
export const PingReviewers = (names: readonly string[], reviewLink: string) => ({
tag: "pinging-reviewers",
status: txt`
|🔔 ${names.map(n => `@${n}`).join(" ")} — please [review this PR](${reviewLink}) in the
|🔔 Pinging owners — please [review this PR](${reviewLink}) in the
fregante marked this conversation as resolved.
Show resolved Hide resolved
next few days. Be sure to explicitly select **\`Approve\`** or **\`Request Changes\`**
in the GitHub UI so I know what's going on.`
});
Expand Down Expand Up @@ -90,7 +90,7 @@ export const PingReviewersTooMany = (names: readonly string[]) => ({
export const PingStaleReviewer = (reviewedAbbrOid: string, reviewers: string[]) => ({
tag: `stale-ping-${sha256(reviewers.join("-")).substr(0, 6)}-${reviewedAbbrOid}`,
status: txt`
|@${reviewers.join(", @")} Thank you for reviewing this PR! The author has pushed new
|**${reviewers.join(", ")}** Thank you for reviewing this PR! The author has pushed new
Copy link
Author

Choose a reason for hiding this comment

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

This is a way to mention people without actuating pinging them. It could be used anywhere.

commits since your last review. Could you take another look and submit a fresh review?`
});

Expand All @@ -106,8 +106,7 @@ export const OfferSelfMerge = deletedWhenNotPresent("merge-offer", tag =>
|> Ready to merge
|
|and I'll merge this PR almost instantly. Thanks for helping out! :heart:
|${otherOwners.length === 0 ? "" : `
|(${otherOwners.map(o => "@" + o).join(", ")}: you can do this too.)`}`}));
|${otherOwners.length === 0 ? "" : `(Any owner can do this too.)`}`}));
Copy link
Member

Choose a reason for hiding this comment

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

Instead of pinging nobody, this should probably ping those reviewers which are also owners.

Copy link
Author

Choose a reason for hiding this comment

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

After the first mention in PingReviewers, every owner is already subscribed and will receive a notification for each commit and comment.

I think this is easier to reason about if you think @user as a Subscribe @user to this PR rather than Ping @user. Do you want to subscribe them over and over?? That's the problematic part.

There should be one subscription, and then GitHub takes care of successive notifications already.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree that it is "over and over" to remind everyone to actually merge the PR.

Copy link
Author

Choose a reason for hiding this comment

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

Mentions are not reminders, they're subscriptions. GitHub already reminds you after the subscription. A mention offers no improvement over no mention if you're already subscribed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't fully agree. Explicit mentions are delivered to a different email address than subscriptions, a fact that some people use to filter their inboxes. Not to mention the notification page, which splits them apart:

image

So, I think that there is still nuance here, moreso than the simple "is or isn't subscribed". Someone could reasonably use "mentioned" as being a higher priority than "subscribed".

But, I also don't have a strong opinion. But, I also have not seen anyone complain about this whole thing outside of your linked issue either.

Copy link
Author

Choose a reason for hiding this comment

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

Please believe when I say that mentions are subscriptions.

Explicit mentions are delivered to a different email address than subscriptions

No.

Look at these two emails: one has a mention, the other is a followup notification. They have identical to/cc fields.

Further mentions or lack of mentions do no change that, unless it's a new "subscription" reason, like "assigned" or "review requested", in which case that sticks around.

Not to mention the notification page, which splits them apart

No. Same exact thing here. Once you've been mentioned, new notifications will continue to appear in the "Mentioned" filter.

… because, once again, GitHub thinks of "subscription reason", not "notification reason"

But, I also don't have a strong opinion

💙

I also have not seen anyone complain

It looks me months to find this bot. Most people will just block the repo or just asked to be removed altogether.

Heck I opened this issue in April 2023 and we're only discussing it now, which option is easier for people? 😃

Copy link
Member

Choose a reason for hiding this comment

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

No. Same exact thing here. Once you've been mentioned, new notifications will continue to appear in the "Mentioned" filter.

From one of your pings:

Screenshot_20240127-045531

Then the email for the above message:

Screenshot_20240127-045708

Notice no "mentioned" the second time. It's possible that the merge event behaves inconsistently.

Most people will just block the repo or just asked to be removed altogether.

That's the wrong logical direction; maybe all annoyed people remove themselves, but not all people who remove themselves do so due to pings specifically.

Copy link
Author

Choose a reason for hiding this comment

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

@jakebailey did you unsubscribe from this thread between January 19 and January 26? I just posted screenshots that show the opposite behavior, so either you unsubscribed, or the behavior isn't really reliable.

Either way the question is simple, I won't continue to debate it:

  • do you want to bother people who clicked Unsubscribe?

Because that's not great netiquette, also technically violating CAN-SPAM

Copy link
Member

Choose a reason for hiding this comment

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

did you unsubscribe from this thread between January 19 and January 26?

No, I did not. Thinking about it, though

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I am not against the change, but just trying to explain that there is nuance in a workflow change like this.

I'm just not really understanding the antagonism here 😞


export const WaitUntilMergeIsOK = (user: string, abbrOid: string, uri: string, mainCommentID: number | undefined) => ({
// at most one reminder per update
Expand All @@ -128,7 +127,7 @@ export const RemindPeopleTheyCanUnblockPR = (user: string, approvalUsers: string
status: txt`
|:hourglass_flowing_sand: Hi @${user},
|
|It's been a few days since this PR was approved by ${approvalUsers.join(", ")} and we're waiting for
|It's been a few days since this PR was approved and we're waiting for
Copy link
Author

Choose a reason for hiding this comment

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

Actually I think this was already non-pinging, so maybe this change isn't necessary

Copy link
Member

Choose a reason for hiding this comment

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

Correct, these are just usernames.

${ciPassing ? `a DT maintainer to give a review`
: `you to fix the test failures and then for a maintainer approval`}.
|
Expand Down