-
Notifications
You must be signed in to change notification settings - Fork 444
feat: Suggest closest command on typo #4367
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -989,4 +989,125 @@ namespace mamba::util | |||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /************************************** | ||||||||||||||||||||||||||||||
| * Implementation of match / suggestion functions * | ||||||||||||||||||||||||||||||
| **************************************/ | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| namespace | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| // Total number of matched characters between strings `a` and `b`. | ||||||||||||||||||||||||||||||
| // Following Python's difflib.SequenceMatcher algorithm : | ||||||||||||||||||||||||||||||
| // Find the longest common contiguous block, then recurse on the left and right of the | ||||||||||||||||||||||||||||||
| // block. | ||||||||||||||||||||||||||||||
| auto ratcliff_obershelp_matches(std::string_view a, std::string_view b) -> std::size_t | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| if (a.empty() || b.empty()) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| return 0; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Longest matching block via difflib DP. | ||||||||||||||||||||||||||||||
| std::size_t best_a = 0; | ||||||||||||||||||||||||||||||
| std::size_t best_b = 0; | ||||||||||||||||||||||||||||||
| std::size_t best_size = 0; | ||||||||||||||||||||||||||||||
| std::vector<std::size_t> j2len(b.size(), 0); | ||||||||||||||||||||||||||||||
| for (std::size_t i = 0; i < a.size(); ++i) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| std::vector<std::size_t> new_j2len(b.size(), 0); | ||||||||||||||||||||||||||||||
| for (std::size_t j = 0; j < b.size(); ++j) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| if (a[i] != b[j]) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| const std::size_t k = (j == 0 ? 0 : j2len[j - 1]) + 1; | ||||||||||||||||||||||||||||||
| new_j2len[j] = k; | ||||||||||||||||||||||||||||||
| if (k > best_size) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| best_a = i - k + 1; | ||||||||||||||||||||||||||||||
| best_b = j - k + 1; | ||||||||||||||||||||||||||||||
| best_size = k; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| j2len = std::move(new_j2len); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (best_size == 0) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| return 0; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const std::size_t left = ratcliff_obershelp_matches( | ||||||||||||||||||||||||||||||
| a.substr(0, best_a), | ||||||||||||||||||||||||||||||
| b.substr(0, best_b) | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| const std::size_t right = ratcliff_obershelp_matches( | ||||||||||||||||||||||||||||||
| a.substr(best_a + best_size), | ||||||||||||||||||||||||||||||
| b.substr(best_b + best_size) | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| return best_size + left + right; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| auto similarity_ratio(std::string_view a, std::string_view b) -> double | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| const std::size_t total = a.size() + b.size(); | ||||||||||||||||||||||||||||||
| if (total == 0) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| return 1.0; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| const std::size_t matches = ratcliff_obershelp_matches(a, b); | ||||||||||||||||||||||||||||||
| return (2.0 * static_cast<double>(matches)) / static_cast<double>(total); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| auto closest_matches( | ||||||||||||||||||||||||||||||
| std::string_view input, | ||||||||||||||||||||||||||||||
| const std::vector<std::string>& candidates, | ||||||||||||||||||||||||||||||
| double cutoff, | ||||||||||||||||||||||||||||||
| std::size_t max_results | ||||||||||||||||||||||||||||||
| ) -> std::vector<std::string> | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| struct Scored | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| double ratio; | ||||||||||||||||||||||||||||||
| const std::string* value; | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Mirror difflib.get_close_matches() implementation. | ||||||||||||||||||||||||||||||
| std::vector<Scored> scored; | ||||||||||||||||||||||||||||||
| scored.reserve(candidates.size()); | ||||||||||||||||||||||||||||||
| for (const auto& candidate : candidates) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| const double ratio = similarity_ratio(input, candidate); | ||||||||||||||||||||||||||||||
| if (ratio >= cutoff) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| scored.push_back({ ratio, &candidate }); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+1079
to
+1087
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neatpick: We could make use of C++20 here. namespace views = std::ranges::views;
auto scored_view = candidates
| views::transform(
[&](const std::string& candidate) -> Scored
{ return { similarity_ratio(input, candidate), &candidate }; }
)
| views::filter([cutoff](const Scored& s) { return s.ratio >= cutoff; });
// TODO(C++23): std::ranges::to<std::vector>
auto scored = std::vector<Scored>(scored_view.begin(), scored_view.end()); |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // difflib returns the n largest (ratio, candidate) pairs, so ties | ||||||||||||||||||||||||||||||
| // are broken by the larger candidate string. | ||||||||||||||||||||||||||||||
| std::stable_sort( | ||||||||||||||||||||||||||||||
| scored.begin(), | ||||||||||||||||||||||||||||||
| scored.end(), | ||||||||||||||||||||||||||||||
| [](const Scored& a, const Scored& b) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| if (a.ratio != b.ratio) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| return a.ratio > b.ratio; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return *a.value > *b.value; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| std::vector<std::string> results; | ||||||||||||||||||||||||||||||
| const std::size_t count = std::min(max_results, scored.size()); | ||||||||||||||||||||||||||||||
| results.reserve(count); | ||||||||||||||||||||||||||||||
| for (std::size_t i = 0; i < count; ++i) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| results.push_back(*scored[i].value); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return results; | ||||||||||||||||||||||||||||||
|
Comment on lines
+1104
to
+1111
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also neatpick.
Suggested change
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -605,5 +605,170 @@ namespace | |||||||||||||||||||
| REQUIRE(concat_dedup_splits("test/chan", "chan/foo", "//") == "test/chan//chan/foo"); | ||||||||||||||||||||
| REQUIRE(concat_dedup_splits("test/chan", "chan/foo", '/') == "test/chan/foo"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| TEST_CASE("similarity_ratio") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| SECTION("Identical and empty strings have ratio one") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(similarity_ratio("", "") == Catch::Approx(1.0)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("install", "install") == Catch::Approx(1.0)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("No common characters have ratio zero") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(similarity_ratio("abc", "xyz") == Catch::Approx(0.0)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("install", "remove") == Catch::Approx(0.0)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Symmetry: ratio is the same regardless of order") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE( | ||||||||||||||||||||
| similarity_ratio("repoquery", "reqoquery") | ||||||||||||||||||||
| == Catch::Approx(similarity_ratio("reqoquery", "repoquery")) | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Known difflib reference values") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(similarity_ratio("instal", "install") == Catch::Approx(0.923077).epsilon(1e-4)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("lsit", "list") == Catch::Approx(0.75)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("inof", "info") == Catch::Approx(0.75)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("ifno", "info") == Catch::Approx(0.75)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("rnu", "run") == Catch::Approx(0.666667).epsilon(1e-4)); | ||||||||||||||||||||
| REQUIRE( | ||||||||||||||||||||
| similarity_ratio("activ", "activate") == Catch::Approx(0.769231).epsilon(1e-4) | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| REQUIRE( | ||||||||||||||||||||
| similarity_ratio("reqoquery", "repoquery") == Catch::Approx(0.888889).epsilon(1e-4) | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| REQUIRE( | ||||||||||||||||||||
| similarity_ratio("whoneds", "whoneeds") == Catch::Approx(0.933333).epsilon(1e-4) | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("kitten", "sitting") == Catch::Approx(0.615385).epsilon(1e-4)); | ||||||||||||||||||||
| REQUIRE(similarity_ratio("ab", "abcdef") == Catch::Approx(0.5)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Case Sensitivity: comparison is byte-exact") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(similarity_ratio("List", "list") == Catch::Approx(0.75)); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Ratio is bounded between 0 and 1") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const char* words[] = { "install", "lst", "repoquery", "", "x", "activate" }; | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer:
Suggested change
|
||||||||||||||||||||
| for (const auto* w1 : words) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| for (const auto* w2 : words) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const double ratio = similarity_ratio(w1, w2); | ||||||||||||||||||||
| REQUIRE(ratio >= 0.0); | ||||||||||||||||||||
| REQUIRE(ratio <= 1.0); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| TEST_CASE("closest_matches") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const std::vector<std::string> commands = { | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
| "activate", "auth", "clean", "config", "create", "env", | ||||||||||||||||||||
| "info", "install", "list", "package", "ps", "remove", | ||||||||||||||||||||
| "repoquery", "run", "search", "shell", "update", | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("A close typo returns the intended command first") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // difflib.get_close_matches("instal", commands) == ["install", "list"]. | ||||||||||||||||||||
| const auto matches = closest_matches("instal", commands); | ||||||||||||||||||||
| REQUIRE(matches.size() == 2); | ||||||||||||||||||||
| REQUIRE(matches.front() == "install"); | ||||||||||||||||||||
| REQUIRE(matches[1] == "list"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Transpositions are matched (parity with conda/difflib)") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(closest_matches("lsit", commands).front() == "list"); | ||||||||||||||||||||
| REQUIRE(closest_matches("lnfo", commands).front() == "info"); | ||||||||||||||||||||
| REQUIRE(closest_matches("ifno", commands).front() == "info"); | ||||||||||||||||||||
| REQUIRE(closest_matches("rnu", commands).front() == "run"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Truncated prefixes are matched") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // "active" -> "activate" has ratio 0.77>= 0.6. | ||||||||||||||||||||
| REQUIRE(closest_matches("active", commands).front() == "activate"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("repoquery -> repoquery") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const auto matches = closest_matches("Repoquery", commands); | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add more test cases for cases with upper case letter to specify edge-cases? E.g. Should the similarity ratio of "Repoquery" and "repoquery" be one? |
||||||||||||||||||||
| REQUIRE_FALSE(matches.empty()); | ||||||||||||||||||||
| REQUIRE(matches.front() == "repoquery"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("An exact match returns itself first") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(closest_matches("list", commands).front() == "list"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Nonsense below the cutoff yields no suggestion") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(closest_matches("zzzzzzzz", commands).empty()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Short, dissimilar tokens that Levenshtein would wrongly match are rejected. | ||||||||||||||||||||
| REQUIRE(closest_matches("xq", commands).empty()); | ||||||||||||||||||||
| REQUIRE(closest_matches("qwerty", commands).empty()); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Results are ordered by decreasing ratio") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const auto matches = closest_matches("instal", commands, 0.6, 5); | ||||||||||||||||||||
| REQUIRE(matches.size() >= 2); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| double previous = 1.0; | ||||||||||||||||||||
| for (const auto& m : matches) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| const double ratio = similarity_ratio(m, "instal"); | ||||||||||||||||||||
| REQUIRE(ratio <= previous + 1e-9); | ||||||||||||||||||||
| previous = ratio; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+728
to
+734
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("max_results caps the number of suggestions") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // A permissive cutoff matches several commands; cap to 2. | ||||||||||||||||||||
| const auto matches = closest_matches("in", commands, 0.1, 2); | ||||||||||||||||||||
| REQUIRE(matches.size() <= 2); | ||||||||||||||||||||
|
Comment on lines
+739
to
+741
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("The cutoff controls how permissive matching is") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // With the default cutoff, "in" only matches "info" (difflib parity). | ||||||||||||||||||||
| const auto strict = closest_matches("in", commands); | ||||||||||||||||||||
| REQUIRE(strict.size() == 1); | ||||||||||||||||||||
| REQUIRE(strict.front() == "info"); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // A very low cutoff lets many more candidates through. | ||||||||||||||||||||
| const auto loose = closest_matches("in", commands, 0.1, 100); | ||||||||||||||||||||
| REQUIRE(loose.size() > strict.size()); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Ties are broken by the larger candidate string (difflib nlargest parity)") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // "ybc" has ratio 2/3 with both "abc" and "xbc"; difflib's nlargest breaks the | ||||||||||||||||||||
| // tie in favour of the lexicographically larger candidate. | ||||||||||||||||||||
| const std::vector<std::string> tie_cands = { "abc", "xbc" }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const auto matches = closest_matches("ybc", tie_cands, 0.6, 2); | ||||||||||||||||||||
| REQUIRE(matches.size() == 2); | ||||||||||||||||||||
| REQUIRE(matches[0] == "xbc"); | ||||||||||||||||||||
| REQUIRE(matches[1] == "abc"); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| SECTION("Empty candidate list yields no matches") | ||||||||||||||||||||
| { | ||||||||||||||||||||
| REQUIRE(closest_matches("list", {}).empty()); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } // namespace mamba | ||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we relax this test to be case insensitive?