Pick reviewer who is not previous assignee when r? group #1958

xizheyin · 2025-04-24T13:06:29Z

This PR implements similar functionality to the reroll, and based on the discussion #triagebot > @rustbot reroll on zulip, it is not necessary for us to implement a separate reroll.

I optimized the logic for r? group. Each time an assignee is set, it is stored in the database, and when we use an r? group, such as r? compiler, I will take out the previously assigned reviewer from the database to avoid to assign a previous assignee.

However. the use of r? @reviewer to specify a specific assignee does not exclude the previous assignee, as this is explicitly specified.

Kobzol · 2025-04-24T13:11:55Z

Thanks for the PR! I think that we should first discuss the design though. I'm not sure if rerolling based on the diff is the most intuitive solution in all cases. If someone did r? compiler before, then rerolling should IMO select someone else from the compiler team/adhoc group, not determine the reviewer based on the file diff. We might thus have to store some per-issue data to remember that a team was previously requested for a review.

I opened a topic about this on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/224082-triagebot/topic/.40rustbot.20reroll/with/514122139.

xizheyin · 2025-04-25T06:30:51Z

I modified the logic:
Only when r? group, we will select a reviewer that has not been previously ASSIGNED. note that this won't work with r? @username because we explicitly specify the name.

I'll comment my changes in the code.

src/handlers/assign.rs

xizheyin · 2025-04-25T06:33:41Z

src/handlers/assign.rs

+                    // TODO: only NoReviewer can be reached here!
                    | e @ FindReviewerError::ReviewerIsPrAuthor { .. }
                    | e @ FindReviewerError::ReviewerAlreadyAssigned { .. }
-                    | e @ FindReviewerError::ReviewerOnVacation { .. },
+                    | e @ FindReviewerError::ReviewerOnVacation { .. }
+                    | e @ FindReviewerError::ReviewerPreviouslyAssigned { .. },


It's worth noting here. Should we use unreachable!() except NoReviewer?

xizheyin · 2025-04-25T06:35:58Z

src/handlers/assign.rs

+        let previous_reviewer_names = get_previous_reviewer_names(ctx, issue).await;
+        let is_previously_assigned = previous_reviewer_names.contains(&candidate);


It is used to know if the candidate is a previous reviewer.

xizheyin · 2025-04-25T06:37:24Z

src/handlers/assign.rs

+            } else if expansion_happened && is_previously_assigned {
+                // **Only** when r? group is expanded, we consider the reviewer previously assigned
+                // `r? @reviewer` will not consider the reviewer previously assigned
+                Some(FindReviewerError::ReviewerPreviouslyAssigned {
+                    username: candidate.clone(),
+                })


I've added logic here to determine if it's a previous Reviewer here only when r? group (expansion happened).

xizheyin · 2025-04-25T06:39:32Z

src/handlers/assign.rs

+
+async fn get_previous_reviewer_names(ctx: &Context, issue: &Issue) -> HashSet<String> {
+    if cfg!(test) {
+        return HashSet::new();
+    }
+
+    // Get the state of the warnings for this PR in the database.
+    let mut db = ctx.db.get().await;
+    let state: IssueData<'_, Reviewer> =
+        match IssueData::load(&mut db, &issue, PREVIOUS_REVIEWER_KEY).await {
+            Ok(state) => state,
+            Err(_) => return HashSet::new(),
+        };
+
+    state.data.names
+}


This corresponds to set_assignee, which stores, and this function is used to read from the database. cfg!(test) returns null in order to pass the tests.

xizheyin · 2025-04-25T06:41:56Z

src/handlers/assign/tests/tests_candidates.rs

        let names: Vec<_> = names.iter().map(|n| n.to_string()).collect();
        match (
-            candidate_reviewers_from_names(&self.teams, &self.config, &self.issue, &names),
+            candidate_reviewers_from_names(&ctx, &self.teams, &self.config, &self.issue, &names)
+                .await,


Change the tests into async because candidate_reviewers_from_names is async.

ehuss · 2025-04-25T14:37:06Z

Can you please update the description with a detailed explanation of what is changing. From the title it is not clear to me what this is doing.

Kobzol

Now that I think about it, remembering the previous requested name (r? name) would be required for rustbot reroll, but it shouldn't be required at all when we are just modifying the request logic to skip the existing reviewer.

In other words, you can get the current assignee directly from the issue passed to the handler. Then we should modify the assignment logic to ignore the previous assignee if a team (not a specific user) was requested.

Kobzol · 2025-04-25T15:02:04Z

src/handlers/assign.rs

@@ -678,7 +711,7 @@ async fn find_reviewer_from_names(
        }
    }

-    let candidates = candidate_reviewers_from_names(teams, config, issue, names)?;
+    let candidates = candidate_reviewers_from_names(ctx, teams, config, issue, names).await?;


Note that #1947 completely refactors these tests and provides them access to an actual database, so it might be better to base the code on top of that.

Okay, when #1947 is merged, I'll rewrite the test.

xizheyin · 2025-04-25T15:25:46Z

Can you please update the description with a detailed explanation of what is changing. From the title it is not clear to me what this is doing.

As discussed in #triagebot > @rustbot reroll @ 💬, I optimized the logic for r? group. Each time an assignee is set, it is stored in the database, and when we use an r? group, such as r? compiler, I will take out the previously assigned reviewer from the database to avoid to assign a previous assignee.

However. the use of r? @reviewer to specify a specific assignee does not exclude the previous assignee, as this is explicitly specified.

xizheyin · 2025-04-25T15:35:52Z

In other words, you can get the current assignee directly from the issue passed to the handler.

issue.assignees seems to only return the current assignees, it may need the database if we want to get all the previous assignees of the issue.

Kobzol · 2025-04-25T16:27:46Z

Hmm, I was thinking that we only ignore the current reviewer, not all previously assigned reviewers 🤔 I'll ask others on Zulip.

ehuss · 2025-04-25T16:32:09Z

As discussed in #triagebot > @rustbot reroll @ 💬, I optimized the logic for r? group. Each time an assignee is set, it is stored in the database, and when we use an r? group, such as r? compiler, I will take out the previously assigned reviewer from the database to avoid to assign a previous assignee.

However. the use of r? @reviewer to specify a specific assignee does not exclude the previous assignee, as this is explicitly specified.

OK, can the description be updated to include that? Or, if the design hasn't been concluded, can this be marked as a draft until it is ready?

Also, this doesn't seem like it would close #1762, would it? That seems like a different thing of the ability to say "I want triagebot to pick someone else".

Kobzol · 2025-04-25T16:33:28Z

FWIW, we have been discussing this in https://rust-lang.zulipchat.com/#narrow/channel/224082-triagebot/topic/.40rustbot.20reroll/with/514415765, and me and Jieyou Xu thought that having an explicit reroll command might not be needed if we make this change to the assignment logic.

rustbot · 2025-04-25T17:08:39Z

Failed to set assignee to group: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2025-04-25T17:08:40Z

Failed to set assignee to group: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2025-04-25T17:08:41Z

Failed to set assignee to tgross35: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2025-04-25T17:08:42Z

Failed to set assignee to reviewer: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

This commit optimized the logic for `r? group`. Each time an assignee is set, it is stored in the database, and when we use an r? group, such as `r? compiler`, it will take out the previously assigned reviewers from the database to avoid to assign a previous assignee. Signed-off-by: xizheyin <[email protected]>

xizheyin · 2025-05-09T10:08:51Z

I rebase this PR to the newest version of master branch that is compatible with the latest test_candidates.rs.

Kobzol · 2025-05-11T07:39:39Z

Hi, just wanted to note that I'll be at https://rustweek.org/, so I might not be able to take a look at this for a few days, but it's at the top of my TODO list :)

xizheyin · 2025-05-11T09:03:46Z

It looks great. Have fun! I don't think the demand for this pr is so urgent.

Kobzol

Looks good, thank you. Could you please also add some tests? You could add a method to AssignCtx that will store a set of previously assigned names into the DB, and then check that these names will be ignored when expanded from a group/team, and will not be ignored when requested directly.

Kobzol · 2025-05-20T07:06:39Z

src/handlers/assign.rs

@@ -92,9 +93,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str =

 Please choose another assignee.";

+const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewers are assigned before.


Suggested change

const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewers are assigned before.

const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewer(s) were already assigned before.

Kobzol · 2025-05-20T07:16:59Z

src/handlers/assign.rs

+/// State stored in the database
+#[derive(Debug, Clone, PartialEq, Default, serde::Deserialize, serde::Serialize)]
+struct Reviewers {
+    /// List of the last warnings in the most recent comment.


Wrong comment.

Kobzol · 2025-05-20T07:17:07Z

src/handlers/assign.rs

 // Special account that we use to prevent assignment.
 const GHOST_ACCOUNT: &str = "ghost";

+/// Key for the state in the database
+const PREVIOUS_REVIEWER_KEY: &str = "previous-reviewer";


Suggested change

const PREVIOUS_REVIEWER_KEY: &str = "previous-reviewer";

const PREVIOUS_REVIEWERS_KEY: &str = "previous-reviewers";

Kobzol · 2025-05-20T07:17:37Z

src/handlers/assign.rs

@@ -680,6 +710,8 @@ enum FindReviewerError {
    ReviewerIsPrAuthor { username: String },
    /// Requested reviewer is already assigned to that PR
    ReviewerAlreadyAssigned { username: String },
+    /// Requested reviewer is already assigned previously to that PR.


Suggested change

/// Requested reviewer is already assigned previously to that PR.

/// Requested reviewer was already assigned previously to that PR.

Kobzol · 2025-05-20T07:18:00Z

src/handlers/assign.rs

+                write!(
+                    f,
+                    "{}",
+                    REVIEWER_ASSIGNED_BEFORE.replace("{username}", username)


REVIEWER_ASSIGNED_BEFORE doesn't seem to contain {username}? It shouldn't need any replacement.

Kobzol · 2025-05-20T07:21:20Z

src/handlers/assign.rs

@@ -963,6 +1008,12 @@ async fn candidate_reviewers_from_names<'a>(
                Some(FindReviewerError::ReviewerAlreadyAssigned {
                    username: candidate.clone(),
                })
+            } else if expansion_happend && is_previously_assigned {


You don't need to check expansion_happened here. We should decide per-reviewer basis if they were expanded or not. It's enough to check reviewer_candidate.origin here.

xizheyin force-pushed the reroll branch from 8e0e906 to bb75bca Compare April 25, 2025 05:52

xizheyin changed the title ~~Add @rustbot reroll to pick a new reviewer~~ Pick reviewer which was not assigned before **only** when r? group Apr 25, 2025

xizheyin force-pushed the reroll branch from bb75bca to 5627a3c Compare April 25, 2025 06:29

xizheyin force-pushed the reroll branch from 5627a3c to 4c97007 Compare April 25, 2025 06:40

xizheyin commented Apr 25, 2025

View reviewed changes

Kobzol reviewed Apr 25, 2025

View reviewed changes

xizheyin changed the title ~~Pick reviewer which was not assigned before **only** when r? group~~ Pick reviewer who is not previous assignee when r? group Apr 25, 2025

xizheyin force-pushed the reroll branch 5 times, most recently from cbbb5af to b0574b6 Compare May 9, 2025 09:43

xizheyin marked this pull request as draft May 9, 2025 09:47

xizheyin force-pushed the reroll branch from b0574b6 to bb063a7 Compare May 9, 2025 10:01

xizheyin force-pushed the reroll branch from bb063a7 to 5692b37 Compare May 9, 2025 10:04

xizheyin marked this pull request as ready for review May 9, 2025 10:07

Kobzol reviewed May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pick reviewer who is not previous assignee when r? group #1958

Pick reviewer who is not previous assignee when r? group #1958

xizheyin commented Apr 24, 2025 •

edited

Loading

Kobzol commented Apr 24, 2025

xizheyin commented Apr 25, 2025

xizheyin Apr 25, 2025

xizheyin Apr 25, 2025

xizheyin Apr 25, 2025

xizheyin Apr 25, 2025

xizheyin Apr 25, 2025

ehuss commented Apr 25, 2025

Kobzol left a comment

Kobzol Apr 25, 2025

xizheyin Apr 25, 2025

xizheyin commented Apr 25, 2025

xizheyin commented Apr 25, 2025

Kobzol commented Apr 25, 2025

ehuss commented Apr 25, 2025

Kobzol commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

xizheyin commented May 9, 2025

Kobzol commented May 11, 2025

xizheyin commented May 11, 2025

Kobzol left a comment •

edited

Loading

Kobzol May 20, 2025

Kobzol May 20, 2025

Kobzol May 20, 2025

Kobzol May 20, 2025

Kobzol May 20, 2025

Kobzol May 20, 2025

		let previous_reviewer_names = get_previous_reviewer_names(ctx, issue).await;
		let is_previously_assigned = previous_reviewer_names.contains(&candidate);

		@@ -92,9 +93,23 @@ const REVIEWER_ALREADY_ASSIGNED: &str =

		Please choose another assignee.";

		const REVIEWER_ASSIGNED_BEFORE: &str = "Requested reviewers are assigned before.

	const PREVIOUS_REVIEWER_KEY: &str = "previous-reviewer";
	const PREVIOUS_REVIEWERS_KEY: &str = "previous-reviewers";

	/// Requested reviewer is already assigned previously to that PR.
	/// Requested reviewer was already assigned previously to that PR.

Pick reviewer who is not previous assignee when r? group #1958

Are you sure you want to change the base?

Pick reviewer who is not previous assignee when r? group #1958

Conversation

xizheyin commented Apr 24, 2025 • edited Loading

Kobzol commented Apr 24, 2025

xizheyin commented Apr 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Apr 25, 2025

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xizheyin commented Apr 25, 2025

xizheyin commented Apr 25, 2025

Kobzol commented Apr 25, 2025

ehuss commented Apr 25, 2025

Kobzol commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

rustbot commented Apr 25, 2025

xizheyin commented May 9, 2025

Kobzol commented May 11, 2025

xizheyin commented May 11, 2025

Kobzol left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xizheyin commented Apr 24, 2025 •

edited

Loading

Kobzol left a comment •

edited

Loading