Skip to content

Only post suppressed assignment warning when assignment succeeds #2023

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

Merged
merged 1 commit into from
May 29, 2025

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented May 29, 2025

To avoid this.

@Kobzol Kobzol requested a review from Urgau May 29, 2025 06:56
Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

Make sense.

@Urgau Urgau added this pull request to the merge queue May 29, 2025
Merged via the queue into rust-lang:master with commit d965575 May 29, 2025
3 checks passed
@Kobzol Kobzol deleted the assignment-suppressed-error-fix branch May 29, 2025 09:44
Comment on lines +315 to 316
r"`{username}` is not on the review rotation at the moment.
They may take a while to respond.
Copy link
Contributor

@apiraino apiraino May 29, 2025

Choose a reason for hiding this comment

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

So now we emit a message that a reviewer not on rotation could take a while to respond? What is the reasoning behind this?

In my mind, if I am off-rotation, I just don't want to set expectations about when or if I'll be reviewing again. A message suggesting that "could take a while" it's putting a burden on the person off-rotation.

Can we just make it so, that in case of r? someone the bot just suggests to re-roll to someone else - without doing anything else and let the person issueing the command take a sensible decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, pretty much everyone that was discussing this on Zulip (in https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Review.20queue.20tracking.20in.20triagebot/with/521048334 and also some other topics) mentioned that they want r? to always work. So that's the rationale for making it work even if the person if off rotation or at their review limit.

The warning just tells the person who requested the review that the requested reviewer is off rotation or at their review limit. Then they can decide to reroll to someone else.

We can modify the warning message to say something like "you may consider rerolling".

Copy link
Contributor Author

@Kobzol Kobzol May 29, 2025

Choose a reason for hiding this comment

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

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 could make this behavior configurable though, that shouldn't be a problem. And allow people to forbid r? <user> if they are off rotation.

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.

3 participants