-
Notifications
You must be signed in to change notification settings - Fork 44
Allow owners to unsubscribe from some PRs #461
base: master
Are you sure you want to change the base?
Conversation
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.
There are probably more messages where the ping should be dropped, like StalenessComment
@@ -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 |
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.
This is a way to mention people without actuating pinging them. It could be used anywhere.
@@ -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 |
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.
Actually I think this was already non-pinging, so maybe this change isn't necessary
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.
Correct, these are just usernames.
@@ -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.)`}`})); |
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.
Instead of pinging nobody, this should probably ping those reviewers which are also owners.
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.
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.
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 disagree that it is "over and over" to remind everyone to actually merge the PR.
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.
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.
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 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:
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.
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.
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? 😃
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.
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:
Then the email for the above message:
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.
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.
@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
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.
did you unsubscribe from this thread between January 19 and January 26?
No, I did not. Thinking about it, though
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.
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 😞
Most likely you're going to need to run something like this to fix the tests, which snapshot replies to PRs. $ AUTH_TOKEN=$(gh auth token) npm run update-all-fixtures (But, after the comment threads are addressed, e.g. the "It's been a few days" does not to be changed) |
No because that exclusively affects people who have unsubscribed, as shown in my other "review" comment. However I think most mentions (ahem re-subscriptions) in I think it's fine for now, later we can see if it's worth making further changes.
Snapshots updated 🫡 Tests pass locally 🟢 PR ready for merge 🚢 Thank you for still being here to discuss this with me 🙏 |
Just to generally chime in here, I'm not wild on these changes I think our biggest problem as DT maintainers is folks with real context not responding to these pings and removing the direct mentions in a bunch of places makes it more likely for them to be ignoring the API response. Like Jake, I have not heard people complain about this outside of the issue you reported and I'm not super convinced that we should be changing this. |
@orta please read the conversation we had so far. People will still be notified. This only affects people who have clicked Unsubscribe after the first notification. The question you have to answer when merging/discarding this PR is: do I want to send a notification to people who specifically clicked Unsubscribe on the current PR? That's all. |
It has been more than a year and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @jakebailey @sandersn @andrewbranch. |
I'm sure this is incomplete, I'm just showing the suggested path forward. If accepted I can complete it.