DefinitelyTyped / dt-mergebot

The bot which handles auto-merging your PRs
https://devblogs.microsoft.com/typescript/changes-to-how-we-manage-definitelytyped/
MIT License
112 stars 44 forks source link

Stop pinging owners over and over for the same PR (allow unsubscribing) #460

Open fregante opened 1 year ago

fregante commented 1 year ago

Pings are necessary to make people aware of a new PR. If they don't take action, every following comment is already delivered to their notification by default. Continuous pinging doesn't do anything extra.

I want owners to have two options:

The second option is not available because the bot keeps bugging everyone in nearly every comment.

fregante commented 9 months ago

@jakebailey @sandersn @andrewbranch

This bot behavior leads to people unsubscribing/blocking DefinitelyTyped, causing them to never contribute to the project again. This is the opposite of what this feature was meant to achieve.

Please review this bot and my open PR, it's a very straightforward change.

jakebailey commented 9 months ago

I'm not sure I agree with your premise. Getting pinged is exactly what a DT owner signed up for; why be an owner if you're not willing to review PRs?

If you're adamant to not re-ping, then either we should always ping initially, or potentially even just check who all has been previously pinged in the PR and only ping new owners (allowing older PRs to continue to ping the most recent people). Your current PR seems to remove the initial ping, which seems incorrect.

(It is of course funny to be pinged here about not wanting to be pinged πŸ˜„)

fregante commented 9 months ago

πŸ™Œ We agree on the base premise, I opened with this:

Pings are necessary to make people aware of a new PR

The issue is not about being pinged, but allowing unsubscribing to each PR.

A single mention is enough to:

Let's say a PR is opened regarding a part of a package that I've never even seen, or that I have no time to review. I want to unsubscribe from that specific PR and not be notified again.

Because let's say Andrew saw this issue and and clicked "Unsubscribe" because he's on vacation, but I'm a bot and I'll mention y'all again:

@jakebailey @sandersn @andrewbranch

…and maybe I'll mention every one again every comment. I bet I'll be blocked after the 3rd mention here. πŸ₯Ή

(It is of course funny to be pinged here about not wanting to be pinged πŸ˜„)

I hope that gets the message across πŸ˜ƒ

fregante commented 9 months ago

check who all has been previously pinged in the PR

The rarity of this subset of PR/owners makes me think it's not worth the extra complexity of this.

jakebailey commented 9 months ago

Because let's say Andrew saw this issue and and clicked "Unsubscribe" because he's on vacation, but I'm a bot and I'll mention y'all again:

Unsubscribing from specific threads is certainly not the method I'd use to silence notifications on vacation. There are much better tools for that. But this is moot.

The rarity of this subset of PR/owners makes me think it's not worth the extra complexity of this.

Yes, that was an over the top suggestion. Simply making it ping only in the first comment would be sufficient.