feat: Suggest closest command on typo - #4367
Conversation
|
Approach
|
|
More testing from my terminal 1. Top level typo 2. Transition typo 3. Truncated 4. Subcommand 5. Alias 6. Flags do not get suggestions 7. Randoms do not get suggestions |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4367 +/- ##
=======================================
Coverage 55.39% 55.40%
=======================================
Files 243 243
Lines 30360 30445 +85
Branches 3253 3273 +20
=======================================
+ Hits 16818 16867 +49
- Misses 13539 13575 +36
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hello @faze-geek, Thank you for this contribution. Although I also think it makes sense for mamba to suggest commands, I wonder whether this issue could better be handled by CLI11 as proposed by @Hind-M with CLIUtils/CLI11#1149. Edit: I guess we could propose this implementation to CLI11 and in the meantime vendor it in mamba. |
|
Hi @jjerphan, thanks for the background. I did go through CLIUtils/CLI11#1149 and see 2 distinct things -
Also I feel the implementation here The design here already anticipates your vendor solution as it's essentially two pure, dependency-free, unit tested functions isolated in Happy to open a CLI11 issue proposing this as a feature and keep working on the mamba fix as well ! Would like to know what you think ? |
+1 for mentioning this solution in CLIUtils/CLI11#1149 and having this implementation in this code-base for now. |
| // A permissive cutoff matches several commands; cap to 2. | ||
| const auto matches = closest_matches("in", commands, 0.1, 2); | ||
| REQUIRE(matches.size() <= 2); |
There was a problem hiding this comment.
| // A permissive cutoff matches several commands; cap to 2. | |
| const auto matches = closest_matches("in", commands, 0.1, 2); | |
| REQUIRE(matches.size() <= 2); | |
| // A permissive cutoff matches several commands; cap to 2. | |
| const auto matches = closest_matches("in", commands, 0.1, 2); | |
| REQUIRE(matches.size() > 2); | |
| const auto matches_capped = closest_matches("in", commands, 0.1, 2); | |
| REQUIRE(matches_capped.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; | ||
| } |
There was a problem hiding this comment.
Can you use std::is_sorted here?
| double previous = 1.0; | |
| for (const auto& m : matches) | |
| { | |
| const double ratio = similarity_ratio(m, "instal"); | |
| REQUIRE(ratio <= previous + 1e-9); | |
| previous = ratio; | |
| } | |
| REQUIRE(std::is_sorted(matches, std::greater<double>)); |
|
|
||
| SECTION("repoquery -> repoquery") | ||
| { | ||
| const auto matches = closest_matches("Repoquery", commands); |
There was a problem hiding this comment.
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?
|
|
||
| SECTION("Ratio is bounded between 0 and 1") | ||
| { | ||
| const char* words[] = { "install", "lst", "repoquery", "", "x", "activate" }; |
There was a problem hiding this comment.
Prefer:
| const char* words[] = { "install", "lst", "repoquery", "", "x", "activate" }; | |
| constexpr std::array<std::string_view, 6> words = { "install", "lst", "repoquery", "", "x", "activate" }; |
|
|
||
| TEST_CASE("closest_matches") | ||
| { | ||
| const std::vector<std::string> commands = { |
There was a problem hiding this comment.
| const std::vector<std::string> commands = { | |
| constexpr std::array<std::string_view, 17> commands = { |
| scored.reserve(candidates.size()); | ||
| for (const auto& candidate : candidates) | ||
| { | ||
| const double ratio = similarity_ratio(input, candidate); | ||
| if (ratio >= cutoff) | ||
| { | ||
| scored.push_back({ ratio, &candidate }); | ||
| } | ||
| } |
There was a problem hiding this comment.
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());| 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; |
There was a problem hiding this comment.
Also neatpick.
| 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; | |
| auto results_view = scored | |
| | views::take(max_results) | |
| | views::transform([](const Scored& s) { return *s.value; }); | |
| // TODO(C++23): std::ranges::to<std::vector> | |
| return std::vector<std::string>(results_view.begin(), results_view.end()); |
| if (a[i] != b[j]) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Should we relax this test to be case insensitive?
Description
Fixes #3856 This PR adds hints when typos are made to subcommands.
Before : Mamba prints only a cryptic CLI11 error message with no hints.
Now : Conda style "Did you mean
<cmd>" suggestion is added.Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.