A new mechanism for showing "did you mean" help. #10872
Conversation
benjyw
force-pushed
the
did_you_mean
branch
from
a6ff830
to
caf0201
Compare
Makes suggestions for unknown goals and unknown flags. The old system for suggesting similar flag names was complex, and didn't handle or display scopes in a user-helpful way. So for example if the user used a global flag in goal scope, we would show a suggestion like: "Unknown flag --foobar, did you mean --foobar". This new implementation handles that case, and moves the error message computation and display out of the options system and into the local_pants_runner.py, which has enough context to do so. [ci skip-rust] [ci skip-build-wheels]
benjyw
force-pushed
the
did_you_mean
branch
from
caf0201
to
0c564b1
Compare
There was a problem hiding this comment.
Excellent improvement, including the suggestion for invalid goals.
It'd be helpful if you could please add some basic smoke unit tests that this works correctly, including the edge case of global flags in the wrong position. This is critical to our UX and I'd hate for us to regress.
|
Also thank you @cosmicexplorer for the initial implementation! That was a huge improvement over what we had before and carried us far. |
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| @@ -56,7 +57,17 @@ def print_hint() -> None: | |||
| elif isinstance(self._help_request, OptionsHelp): | |||
| self._print_options_help() | |||
| elif isinstance(self._help_request, UnknownGoalHelp): | |||
| print("Unknown goals: {}".format(", ".join(self._help_request.unknown_goals))) | |||
| # Only print help and suggestions for the first unknown goal. | |||
| # It gets confusing to try and show suggestions for multiple cases. | |||
There was a problem hiding this comment.
Good move. The user can recursively whittle down unknown goals until the whole command line makes sense.
| class MaybeColor: | ||
| """A mixin to allow classes to optionally colorize their output.""" | ||
|
|
||
| def __init__(self, color: bool) -> None: |
There was a problem hiding this comment.
Would we ever want to make this color configurable? Emacs registers faces (see https://www.gnu.org/software/emacs/manual/html_node/elisp/Defining-Faces.html) which allow e.g. for the user to modify colors if their terminal doesn't interact well with the default. That seems like it would mostly just require turning this into a subsystem.
There was a problem hiding this comment.
Not a change I would expect in this PR.
There was a problem hiding this comment.
I could maybe see wanting that. But I think it would need to be a booststrap subsystem because Rust uses this option too. Right now, bootstrap options require a ton of ceremony and must be global options, so I think we'd want to improve that first.
We'd also need to stop using ansicolors to instead use another library (or honestly just us vendoring the relevant parts; it's a tiny amount of code). We probably need to do this soon anyways, as ansicolors hasn't been updated in some time and will break once 3.10 comes out afaict.
Makes suggestions for unknown goals and unknown flags.
The old system for suggesting similar flag names was clever
but complex, and didn't always handle or display scopes in a
user-helpful way. So for example if the user used a
global flag in goal scope, we would show a suggestion like:
"Unknown flag --foobar on scope blah, did you mean --foobar".
This new implementation handles that case, and moves
the error message computation and display out of the
options system and into the local_pants_runner.py,
which has enough context to get suggested flags without
the gymnastics that the old code had to do.
This new code uses Python's built-in difflib to find similar
strings, which is sufficient for this use-case, and so we
can drop the python-Levenshtein dependency.
[ci skip-rust]
[ci skip-build-wheels]