Implement tab autocomplete for ruff config
#15603
Conversation
|
emit the full first paragraph for completion doc
|
updates:
|
Are you referring to the header like $ ruff config lint.ruff
parenthesize-tuple-in-subscript
extend-markup-names
allowed-markup-calls |
Yes. Here is why I think there is no point in including them. The whole purpose of completion is to allow a user to get to detailed configuration field description without guessing its name. Configuration field groups, like See, how the flow looks at the current commit. I first run Now, here is how tab completion will look if I remove the filter: I am not sure if the first line is helpful. Even if I add something like An important note - if tl;dr; I am happy to remove the filter if you think this is how it is supposed to be, but imho the current behavior gets users to where they actually want with slightly less confusion. |
|
Thanks for providing your thoughts on this. I agree that it might be quicker for a user to get to a specific config key if we skip the plugin header but those are the possible values that can be passed to the |
|
I think it's fine to ignore tables as long as we ignore all table entries because you can't set the table itself (I guess you can, but it's somewhat useless). |
I see, I'm not sure if that kind of heuristics exists, maybe it does within the OptionsMetadata? Regardless, I'm not sure I follow what you mean by "you can't set the table itself" as, from what I understand, |
|
@MichaReiser as per my logic above - when an OptionSet has some documentation, it is kinda helpful to see it among completions...but tbh only when the set is small and this option even fits on the screen. So there are two viable alternatives to resolve this:
I'll follow your lead folks, let me know. |
There was a problem hiding this comment.
@dhruvmanila convinced me. I think showing all values is a great starting point.
Can you add an integration test similar to the tests we have in https://github.com/astral-sh/ruff/blob/c847cad389f202edae868765e690cb7042e88264/crates/ruff/tests/config.rs#L5-L4
crates/ruff_workspace/src/cli.rs
Outdated
| let first_paragraph = doc | ||
| .lines() | ||
| .take_while(|l| !l.trim_end().is_empty()) | ||
| .map(|s| s.replace('"', "'")) |
There was a problem hiding this comment.
Can you explain why replacing all " with ' is necessary? It does seem dangerous to me in the case the doc contains any code examples
There was a problem hiding this comment.
@MichaReiser first of all thanks a LOT for such a thorough review. I almost feel sorry that such a minor change caused so much trouble.
Back to your question: I added it because apparently clap doesn't escape double quotes when generating zsh completions, which breaks them. @dhruvmanila commented about it: #15603 (review)
Also, found clap PR that fixed other escapes: clap-rs/clap#4848 but if you look into the change here: https://github.com/clap-rs/clap/pull/4848/files - you'll see that (a) they effectively do the same, just replace characters (b) PR author forgot double quotes
I do not see any risks, neither security no user friendliness. This would only affect completion help, which most likely be truncated anyway + it is unlikely that the first paragraph of the doc contains code snippets.
There was a problem hiding this comment.
It would be great if you could add an inline comment that summarises what you wrote in this comment. It will help future readers to understand (without guessing) of what's happening here.
crates/ruff_workspace/src/cli.rs
Outdated
| if let Some(arg) = arg { | ||
| error.insert( | ||
| clap::error::ContextKind::InvalidArg, | ||
| clap::error::ContextValue::String(arg.to_string()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Doesn't this result in two errors with we have an argument? Should this be a match? Can you extend your test plan with an example that exercises this code path
There was a problem hiding this comment.
this code was copy-pasted verbatim from the existing completion for rules... it gives this:
I believe without the arg, the for '[OPTION]' will be missing.
I am not a fan of this specific output, but I was trying to stick to whatever was in the code base already.
There was a problem hiding this comment.
I see. I'm still not sure if it is correct because we'll end up inserting two errors in case arg is some. So maybe this should just be an if...else?
There was a problem hiding this comment.
No, I believe this code (which btw Charlie wrote originally) is correct. Clap uses each piece to create one single error. Notice that one part pushes clap::error::ContextKind::InvalidArg and another clap::error::ContextKind::InvalidValue, hence we get one error:
error: invalid value 'blabla' for '[OPTION]'
^^^^^ <--- clap::error::ContextKind::InvalidValue
^^^^^^^^ <--- clap::error::ContextKind::InvalidArg
crates/ruff_workspace/src/cli.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Opaque type for option strings on the command line. |
There was a problem hiding this comment.
I think it would be helpful to mention that this type mainly exists to provide autocompletion by implementing PossibleValues.
Can you add the help output for the config command to your test plan. I want to make sure that clap doesn't render all possible values
Made the change
doing a true completion integration test seems cumbersome, it would involve generating the completion script, sourcing it, and then executing with some magic env vars inside zsh. I can add such a test, but sounds overcomplicated for the task. A somewhat easier way is to commit the completion script itself as a snapshot. That's easy. But whatever path we choose will cause snapshot changes every time anyone changes any option... As a contributor I would have been surprised by this ;-) Fwiw the other args with autocompletion do not have snapshot tests if I am not mistaken |
show all option sets, even without a doc do not show completion values in help
Sorry I missed this. Can you tell me why is that so? In my testing, I can at least see that the Bash completions are being produced correctly. |
| group | ||
| .documentation() | ||
| .unwrap_or("Print a list of available options") | ||
| .to_owned(), |
There was a problem hiding this comment.
I don't think this is the correct fix. The fix would be to update the actual documentation in the codebase. For example, the ruff config lint option has the following as documentation:
ruff/crates/ruff_workspace/src/options.rs
Lines 436 to 446 in 98fccec
The first line will be used as the description for the completion. This is what should be done as well for other options and I'd recommend that this be done in a follow-up PR and not in this PR.
So, for example, considering lint.pydoclint, the following comment:
ruff/crates/ruff_workspace/src/options.rs
Lines 472 to 474 in 98fccec
should be copied on top of PydoclintOptions which will then be used as the completion description.
| fn from(o: OptionString) -> Self { | ||
| o.0 | ||
| } |
There was a problem hiding this comment.
nit: o as a single letter is confusing
| fn from(o: OptionString) -> Self { | |
| o.0 | |
| } | |
| fn from(value: OptionString) -> Self { | |
| value.0 | |
| } |
| Some(Box::new(visitor.into_iter().map(|(name, doc)| { | ||
| let first_paragraph = doc | ||
| .lines() | ||
| .take_while(|l| !l.trim_end().is_empty()) |
There was a problem hiding this comment.
| .take_while(|l| !l.trim_end().is_empty()) | |
| .take_while(|line| !line.trim_end().is_empty()) |
There was a problem hiding this comment.
Looks good, thanks for working on this. I'll let @MichaReiser do the final review.
We should revert the default documentation value and use empty string for now. A follow-up work would be to copy the documentation to the relevant struct.
should be ok to merge |
Summary
Not the most important feature, but hey... was marked as the good first issue ;-) fixes #4551
Unfortunately, looks like clap only generates proper completions for zsh, so this would not make any difference for bash/fish.
Test Plan
❯ target/debug/ruff config -h List or describe the available configuration options Usage: ruff config [OPTIONS] [OPTION] Arguments: [OPTION] Config key to show Options: --output-format <OUTPUT_FORMAT> Output format [default: text] [possible values: text, json] -h, --help Print help Log levels: -v, --verbose Enable verbose logging -q, --quiet Print diagnostics, but nothing else -s, --silent Disable all logging (but still exit with status code "1" upon detecting diagnostics) Global options: --config <CONFIG_OPTION> Either a path to a TOML configuration file (`pyproject.toml` or `ruff.toml`), or a TOML `<KEY> = <VALUE>` pair (such as you might find in a `ruff.toml` configuration file) overriding a specific configuration option. Overrides of individual settings using this option always take precedence over all configuration files, including configuration files that were also specified using `--config` --isolated Ignore all configuration files