fix(forge): suggest the nearest name when a filter value misses
The third bullet of the issue's ask, dropped in the first pass and caught in review: a filter value that does not resolve is a near-miss far more often than an invention, and an error that only lists all 18 available names makes the reader do the diff by eye -- on the one occasion they already know they mistyped something. Thresholded rather than always suggesting the minimum-distance candidate: a wrong suggestion is worse than none, because it invites a second failed attempt at a name that was never there. The bound scales with the needle (a third of its length, capped at 3), so a short name does not match half the repo and a long one still tolerates a typo or two, and an unrelated word falls back to the full list. Tie-break is on length then alphabetical, so the suggestion does not depend on the order the forge happened to return its labels in.
This commit is contained in:
parent
0aa9a854bc
commit
6e64489050
3 changed files with 101 additions and 2 deletions
|
|
@ -145,7 +145,7 @@ pub(crate) fn resolve_ids(all: &[Label], names: &[String]) -> Result<Vec<i64>> {
|
||||||
let available: Vec<&str> = all.iter().filter_map(|l| l.name.as_deref()).collect();
|
let available: Vec<&str> = all.iter().filter_map(|l| l.name.as_deref()).collect();
|
||||||
bail!(
|
bail!(
|
||||||
"unresolved label name(s): {} — available labels: {}",
|
"unresolved label name(s): {} — available labels: {}",
|
||||||
unresolved.join(", "),
|
crate::verbs::with_suggestions(&unresolved, &available),
|
||||||
if available.is_empty() {
|
if available.is_empty() {
|
||||||
"(none)".to_owned()
|
"(none)".to_owned()
|
||||||
} else {
|
} else {
|
||||||
|
|
|
||||||
|
|
@ -100,7 +100,7 @@ pub(crate) fn ensure_filters_resolve(all: &[Milestone], values: &[String]) -> Re
|
||||||
let available: Vec<&str> = all.iter().filter_map(|m| m.title.as_deref()).collect();
|
let available: Vec<&str> = all.iter().filter_map(|m| m.title.as_deref()).collect();
|
||||||
bail!(
|
bail!(
|
||||||
"unresolved milestone(s): {} — available milestones: {}",
|
"unresolved milestone(s): {} — available milestones: {}",
|
||||||
unresolved.join(", "),
|
crate::verbs::with_suggestions(&unresolved, &available),
|
||||||
if available.is_empty() {
|
if available.is_empty() {
|
||||||
"(none)".to_owned()
|
"(none)".to_owned()
|
||||||
} else {
|
} else {
|
||||||
|
|
|
||||||
|
|
@ -172,6 +172,61 @@ pub(crate) fn pct_encode(s: &str) -> String {
|
||||||
out
|
out
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Render an unresolved-name list with a `did you mean` where one fits.
|
||||||
|
///
|
||||||
|
/// A filter value that doesn't resolve is a **near-miss far more often
|
||||||
|
/// than an invention** (`area/opps` for `area/ops`), and an error that
|
||||||
|
/// only lists all 18 available names makes the reader do the diff by eye
|
||||||
|
/// — on the one occasion they already know they mistyped something.
|
||||||
|
pub(crate) fn with_suggestions(unresolved: &[&str], available: &[&str]) -> String {
|
||||||
|
unresolved
|
||||||
|
.iter()
|
||||||
|
.map(|u| match nearest(u, available) {
|
||||||
|
Some(s) => format!("{u} (did you mean \"{s}\"?)"),
|
||||||
|
None => (*u).to_owned(),
|
||||||
|
})
|
||||||
|
.collect::<Vec<_>>()
|
||||||
|
.join(", ")
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Closest candidate to `needle`, when one is close enough to be worth
|
||||||
|
/// suggesting.
|
||||||
|
///
|
||||||
|
/// Thresholded rather than always returning the minimum: **a wrong
|
||||||
|
/// suggestion is worse than none**, because it invites a second failed
|
||||||
|
/// attempt at a name that was never there. The bound scales with the
|
||||||
|
/// needle (a third of its length, capped at 3) so a short name doesn't
|
||||||
|
/// match half the repo and a long one still tolerates a typo or two.
|
||||||
|
fn nearest<'a>(needle: &str, candidates: &[&'a str]) -> Option<&'a str> {
|
||||||
|
let limit = (needle.chars().count() / 3).clamp(1, 3);
|
||||||
|
candidates
|
||||||
|
.iter()
|
||||||
|
.map(|c| (edit_distance(needle, c), *c))
|
||||||
|
.filter(|(d, _)| *d <= limit)
|
||||||
|
// Tie-break on the shorter candidate, then alphabetically, so the
|
||||||
|
// suggestion is stable rather than dependent on the order the
|
||||||
|
// forge happened to return its labels in.
|
||||||
|
.min_by_key(|(d, c)| (*d, c.len(), *c))
|
||||||
|
.map(|(_, c)| c)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Levenshtein distance, two-row DP. Small enough not to justify a
|
||||||
|
/// workspace dependency for the one place it is used.
|
||||||
|
fn edit_distance(a: &str, b: &str) -> usize {
|
||||||
|
let b: Vec<char> = b.chars().collect();
|
||||||
|
let mut prev: Vec<usize> = (0..=b.len()).collect();
|
||||||
|
let mut cur = vec![0_usize; b.len() + 1];
|
||||||
|
for (i, ca) in a.chars().enumerate() {
|
||||||
|
cur[0] = i + 1;
|
||||||
|
for (j, cb) in b.iter().enumerate() {
|
||||||
|
let cost = usize::from(ca != *cb);
|
||||||
|
cur[j + 1] = (prev[j] + cost).min(prev[j + 1] + 1).min(cur[j] + 1);
|
||||||
|
}
|
||||||
|
std::mem::swap(&mut prev, &mut cur);
|
||||||
|
}
|
||||||
|
prev[b.len()]
|
||||||
|
}
|
||||||
|
|
||||||
/// One reviewer's latest verdict on a PR, plus forgejo's `stale` /
|
/// One reviewer's latest verdict on a PR, plus forgejo's `stale` /
|
||||||
/// `dismissed` bits.
|
/// `dismissed` bits.
|
||||||
///
|
///
|
||||||
|
|
@ -303,4 +358,48 @@ mod tests {
|
||||||
assert_eq!(pct_encode("x&y"), "x%26y");
|
assert_eq!(pct_encode("x&y"), "x%26y");
|
||||||
assert_eq!(pct_encode("a/b"), "a%2Fb");
|
assert_eq!(pct_encode("a/b"), "a%2Fb");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn nearest_finds_the_one_character_typo() {
|
||||||
|
let all = ["area/ops", "area/broker", "type/bug"];
|
||||||
|
assert_eq!(super::nearest("area/opps", &all), Some("area/ops"));
|
||||||
|
assert_eq!(super::nearest("type/bugs", &all), Some("type/bug"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn nearest_suggests_nothing_for_an_invention() {
|
||||||
|
// The important half: a wrong suggestion invites a second failed
|
||||||
|
// attempt at a name that was never there, so far-away input must
|
||||||
|
// fall back to "here is everything".
|
||||||
|
let all = ["area/ops", "area/broker", "type/bug"];
|
||||||
|
assert_eq!(super::nearest("frontend", &all), None);
|
||||||
|
assert_eq!(super::nearest("", &all), None);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn nearest_is_stable_when_two_candidates_tie() {
|
||||||
|
// Both are distance 1 from "v3"; the answer must not depend on
|
||||||
|
// the order the forge returned them in.
|
||||||
|
let forward = ["v1", "v2"];
|
||||||
|
let reversed = ["v2", "v1"];
|
||||||
|
assert_eq!(
|
||||||
|
super::nearest("v3", &forward),
|
||||||
|
super::nearest("v3", &reversed)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn with_suggestions_annotates_only_the_near_misses() {
|
||||||
|
let all = ["area/ops", "type/bug"];
|
||||||
|
let rendered = super::with_suggestions(&["area/opps", "frontend"], &all);
|
||||||
|
assert!(
|
||||||
|
rendered.contains(r#"area/opps (did you mean "area/ops"?)"#),
|
||||||
|
"{rendered}"
|
||||||
|
);
|
||||||
|
assert!(rendered.contains("frontend"), "{rendered}");
|
||||||
|
assert!(
|
||||||
|
!rendered.contains(r"frontend (did you mean"),
|
||||||
|
"invented name must not get a suggestion: {rendered}"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue