Skip to content

Commit

Permalink
Merge pull request #1668 from ehuss/better-filter-err
Browse files Browse the repository at this point in the history 
Provide a better error message when r? fails
  • Loading branch information
@Mark-Simulacrum
Mark-Simulacrum authored Nov 2, 2022
2 parents 98afddb + efb7148 commit 987ad86
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 53 deletions.
104 changes: 75 additions & 29 deletions src/handlers/assign.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,11 @@ async fn determine_assignee(
is there maybe a misconfigured group?",
event.issue.global_id()
),
Err(FindReviewerError::NoReviewer(names)) => log::trace!(
"no reviewer could be determined for PR {} with candidate name {names:?}",
Err(
e @ FindReviewerError::NoReviewer { .. }
| e @ FindReviewerError::AllReviewersFiltered { .. },
) => log::trace!(
"no reviewer could be determined for PR {}: {e}",
event.issue.global_id()
),
}
Expand Down Expand Up @@ -547,32 +550,61 @@ pub(super) async fn handle_command(
Ok(())
}

#[derive(Debug)]
#[derive(PartialEq, Debug)]
enum FindReviewerError {
/// User specified something like `r? foo/bar` where that team name could
/// not be found.
TeamNotFound(String),
/// No reviewer could be found. The field is the list of candidate names
/// that were used to seed the selection. One example where this happens
/// is if the given name was for a team where the PR author is the only
/// member.
NoReviewer(Vec<String>),
/// No reviewer could be found.
///
/// This could happen if there is a cyclical group or other misconfiguration.
/// `initial` is the initial list of candidate names.
NoReviewer { initial: Vec<String> },
/// All potential candidates were excluded. `initial` is the list of
/// candidate names that were used to seed the selection. `filtered` is
/// the users who were prevented from being assigned. One example where
/// this happens is if the given name was for a team where the PR author
/// is the only member.
AllReviewersFiltered {
initial: Vec<String>,
filtered: Vec<String>,
},
}

impl std::error::Error for FindReviewerError {}

impl fmt::Display for FindReviewerError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
match self {
FindReviewerError::TeamNotFound(team) => write!(f, "Team or group `{team}` not found.\n\
\n\
rust-lang team names can be found at https://github.com/rust-lang/team/tree/master/teams.\n\
Reviewer group names can be found in `triagebot.toml` in this repo."),
FindReviewerError::NoReviewer(names) => write!(
f,
"Could not determine reviewer from `{}`.",
names.join(",")
),
FindReviewerError::TeamNotFound(team) => {
write!(
f,
"Team or group `{team}` not found.\n\
\n\
rust-lang team names can be found at https://github.com/rust-lang/team/tree/master/teams.\n\
Reviewer group names can be found in `triagebot.toml` in this repo."
)
}
FindReviewerError::NoReviewer { initial } => {
write!(
f,
"No reviewers could be found from initial request `{}`\n\
This repo may be misconfigured.\n\
Use r? to specify someone else to assign.",
initial.join(",")
)
}
FindReviewerError::AllReviewersFiltered { initial, filtered } => {
write!(
f,
"Could not assign reviewer from: `{}`.\n\
User(s) `{}` are either the PR author or are already assigned, \
and there are no other candidates.\n\
Use r? to specify someone else to assign.",
initial.join(","),
filtered.join(","),
)
}
}
}
}
Expand Down Expand Up @@ -609,12 +641,11 @@ fn find_reviewer_from_names(
//
// These are all ideas for improving the selection here. However, I'm not
// sure they are really worth the effort.
match candidates.into_iter().choose(&mut rand::thread_rng()) {
Some(candidate) => Ok(candidate.to_string()),
None => Err(FindReviewerError::NoReviewer(
names.iter().map(|n| n.to_string()).collect(),
)),
}
Ok(candidates
.into_iter()
.choose(&mut rand::thread_rng())
.expect("candidate_reviewers_from_names always returns at least one entry")
.to_string())
}

/// Returns a list of candidate usernames to choose as a reviewer.
Expand All @@ -635,16 +666,22 @@ fn candidate_reviewers_from_names<'a>(
// below will pop from this and then append the expanded results of teams.
// Usernames will be added to `candidates`.
let mut group_expansion: Vec<&str> = names.iter().map(|n| n.as_str()).collect();
// Keep track of which users get filtered out for a better error message.
let mut filtered = Vec::new();
let repo = issue.repository();
let org_prefix = format!("{}/", repo.organization);
// Don't allow groups or teams to include the current author or assignee.
let filter = |name: &&str| -> bool {
let mut filter = |name: &&str| -> bool {
let name_lower = name.to_lowercase();
name_lower != issue.user.login.to_lowercase()
let ok = name_lower != issue.user.login.to_lowercase()
&& !issue
.assignees
.iter()
.any(|assignee| name_lower == assignee.login.to_lowercase())
.any(|assignee| name_lower == assignee.login.to_lowercase());
if !ok {
filtered.push(name.to_string());
}
ok
};

// Loop over groups to recursively expand them.
Expand All @@ -663,7 +700,7 @@ fn candidate_reviewers_from_names<'a>(
group_members
.iter()
.map(|member| member.as_str())
.filter(filter),
.filter(&mut filter),
);
}
continue;
Expand All @@ -683,7 +720,7 @@ fn candidate_reviewers_from_names<'a>(
team.members
.iter()
.map(|member| member.github.as_str())
.filter(filter),
.filter(&mut filter),
);
continue;
}
Expand All @@ -697,5 +734,14 @@ fn candidate_reviewers_from_names<'a>(
candidates.insert(group_or_user);
}
}
Ok(candidates)
if candidates.is_empty() {
let initial = names.iter().cloned().collect();
if filtered.is_empty() {
Err(FindReviewerError::NoReviewer { initial })
} else {
Err(FindReviewerError::AllReviewersFiltered { initial, filtered })
}
} else {
Ok(candidates)
}
}
78 changes: 54 additions & 24 deletions src/handlers/assign/tests/tests_candidates.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,26 @@ fn test_from_names(
config: toml::Value,
issue: serde_json::Value,
names: &[&str],
expected: &[&str],
expected: Result<&[&str], FindReviewerError>,
) {
let (teams, config, issue) = convert_simplified(teams, config, issue);
let names: Vec<_> = names.iter().map(|n| n.to_string()).collect();
let candidates = candidate_reviewers_from_names(&teams, &config, &issue, &names).unwrap();
let mut candidates: Vec<_> = candidates.into_iter().collect();
candidates.sort();
let expected: Vec<_> = expected.iter().map(|x| *x).collect();
assert_eq!(candidates, expected);
match (
candidate_reviewers_from_names(&teams, &config, &issue, &names),
expected,
) {
(Ok(candidates), Ok(expected)) => {
let mut candidates: Vec<_> = candidates.into_iter().collect();
candidates.sort();
let expected: Vec<_> = expected.iter().map(|x| *x).collect();
assert_eq!(candidates, expected);
}
(Err(actual), Err(expected)) => {
assert_eq!(actual, expected)
}
(Ok(candidates), Err(_)) => panic!("expected Err, got Ok: {candidates:?}"),
(Err(e), Ok(_)) => panic!("expected Ok, got Err: {e}"),
}
}

/// Convert the simplified input in preparation for `candidate_reviewers_from_names`.
Expand Down Expand Up @@ -78,7 +89,15 @@ fn circular_groups() {
other = ["compiler"]
);
let issue = generic_issue("octocat", "rust-lang/rust");
test_from_names(None, config, issue, &["compiler"], &[]);
test_from_names(
None,
config,
issue,
&["compiler"],
Err(FindReviewerError::NoReviewer {
initial: vec!["compiler".to_string()],
}),
);
}

#[test]
Expand All @@ -91,7 +110,7 @@ fn nested_groups() {
c = ["a", "b"]
);
let issue = generic_issue("octocat", "rust-lang/rust");
test_from_names(None, config, issue, &["c"], &["nrc", "pnkfelix"]);
test_from_names(None, config, issue, &["c"], Ok(&["nrc", "pnkfelix"]));
}

#[test]
Expand All @@ -102,7 +121,16 @@ fn candidate_filtered_author_only_candidate() {
compiler = ["nikomatsakis"]
);
let issue = generic_issue("nikomatsakis", "rust-lang/rust");
test_from_names(None, config, issue, &["compiler"], &[]);
test_from_names(
None,
config,
issue,
&["compiler"],
Err(FindReviewerError::AllReviewersFiltered {
initial: vec!["compiler".to_string()],
filtered: vec!["nikomatsakis".to_string()],
}),
);
}

#[test]
Expand All @@ -119,7 +147,7 @@ fn candidate_filtered_author() {
config,
issue,
&["compiler"],
&["user1", "user3", "user4"],
Ok(&["user1", "user3", "user4"]),
);
}

Expand All @@ -135,7 +163,7 @@ fn candidate_filtered_assignee() {
{"login": "user1", "id": 1},
{"login": "user3", "id": 3},
]);
test_from_names(None, config, issue, &["compiler"], &["user4"]);
test_from_names(None, config, issue, &["compiler"], Ok(&["user4"]));
}

#[test]
Expand All @@ -155,7 +183,7 @@ fn groups_teams_users() {
config,
issue,
&["team1", "group1", "user3"],
&["t-user1", "t-user2", "user1", "user3"],
Ok(&["t-user1", "t-user2", "user1", "user3"]),
);
}

Expand All @@ -173,14 +201,14 @@ fn group_team_user_precedence() {
config.clone(),
issue.clone(),
&["compiler"],
&["user2"],
Ok(&["user2"]),
);
test_from_names(
Some(teams.clone()),
config.clone(),
issue.clone(),
&["rust-lang/compiler"],
&["user2"],
Ok(&["user2"]),
);
}

Expand All @@ -200,22 +228,22 @@ fn what_do_slashes_mean() {
config.clone(),
issue.clone(),
&["foo/bar"],
&["foo-user"],
Ok(&["foo-user"]),
);
// Since this is rust-lang-nursery, it uses the rust-lang team, not the group.
test_from_names(
Some(teams.clone()),
config.clone(),
issue.clone(),
&["rust-lang/compiler"],
&["t-user1"],
Ok(&["t-user1"]),
);
test_from_names(
Some(teams.clone()),
config.clone(),
issue.clone(),
&["rust-lang-nursery/compiler"],
&["user2"],
Ok(&["user2"]),
);
}

Expand All @@ -227,11 +255,13 @@ fn invalid_org_doesnt_match() {
compiler = ["user2"]
);
let issue = generic_issue("octocat", "rust-lang/rust");
let (teams, config, issue) = convert_simplified(Some(teams), config, issue);
let names = vec!["github/compiler".to_string()];
match candidate_reviewers_from_names(&teams, &config, &issue, &names) {
Ok(x) => panic!("expected err, got {x:?}"),
Err(FindReviewerError::TeamNotFound(_)) => {}
Err(e) => panic!("unexpected error {e:?}"),
}
test_from_names(
Some(teams),
config,
issue,
&["github/compiler"],
Err(FindReviewerError::TeamNotFound(
"github/compiler".to_string(),
)),
);
}

0 comments on commit 987ad86

Please sign in to comment.