Implement tab autocomplete for `ruff config` by mishamsk · Pull Request #15603 · astral-sh/ruff

mishamsk · 2025-01-20T03:46:47Z

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

cargo nextest run
manual test by sourcing completions and then triggering autocomplete:

misha@PandaBook ruff % source <(target/debug/ruff generate-shell-completion zsh)
misha@PandaBook ruff % target/debug/ruff config lin
line-length                                                         -- The line length to use when enforcing long-lines violations
lint                                                                -- Configures how Ruff checks your code.
lint.allowed-confusables                                            -- A list of allowed 'confusable' Unicode characters to ignore
lint.dummy-variable-rgx                                             -- A regular expression used to identify 'dummy' variables, or
lint.exclude                                                        -- A list of file patterns to exclude from linting in addition
lint.explicit-preview-rules                                         -- Whether to require exact codes to select preview rules. Whe
lint.extend-fixable                                                 -- A list of rule codes or prefixes to consider fixable, in ad
lint.extend-ignore                                                  -- A list of rule codes or prefixes to ignore, in addition to
lint.extend-per-file-ignores                                        -- A list of mappings from file pattern to rule codes or prefi
lint.extend-safe-fixes                                              -- A list of rule codes or prefixes for which unsafe fixes sho
lint.extend-select                                                  -- A list of rule codes or prefixes to enable, in addition to
lint.extend-unsafe-fixes                                            -- A list of rule codes or prefixes for which safe fixes shoul
lint.external                                                       -- A list of rule codes or prefixes that are unsupported by Ru
lint.fixable                                                        -- A list of rule codes or prefixes to consider fixable. By de
lint.flake8-annotations                                             -- Print a list of available options
lint.flake8-annotations.allow-star-arg-any                          -- Whether to suppress `ANN401` for dynamically typed `*args`

...

check command help

❯ 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

running original command

❯ target/debug/ruff config
cache-dir
extend
output-format
fix
unsafe-fixes
fix-only
show-fixes
required-version
preview
exclude
extend-exclude
extend-include
force-exclude
include
respect-gitignore
builtins
namespace-packages
target-version
src
line-length
indent-width
lint
format
analyze

github-actions · 2025-01-20T03:55:43Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

dhruvmanila

Thank you for working on this!

I'm seeing this entry at the end of the completion list after I press Tab at the end of ruff config :

crates/ruff_workspace/src/cli.rs

emit the full first paragraph for completion doc

…pe them

mishamsk · 2025-01-20T14:59:00Z

@dhruvmanila

updates:

fixes the erroneous strings at the end of completions (apparently clap has issues with double quotes in help strings, fails to properly escape them => I replaced with single quotes)
removed sets from completions that do not have doc. Even though it would be valid to call config with them, I do not see additional value in polluting the completion. In real-life zsh would "stop" at them anyway, albeit with a trailing dot
as described above, I've switched to passing the entire first paragraph as help string. Doesn't fix the fact that zsh truncates the help to screen width, but probably better than hard-truncation I had before

dhruvmanila · 2025-01-20T15:06:13Z

removed sets from completions that do not have doc. Even though it would be valid to call config with them, I do not see additional value in polluting the completion. In real-life zsh would "stop" at them anyway, albeit with a trailing dot

Are you referring to the header like lint.pyupgrade, lint.ruff, etc. as is in my screenshot above? If so, we should include them even though they don't have documentation because ruff config lint.ruff will list out all the config options:

$ ruff config lint.ruff
parenthesize-tuple-in-subscript
extend-markup-names
allowed-markup-calls

mishamsk · 2025-01-20T18:03:02Z

removed sets from completions that do not have doc. Even though it would be valid to call config with them, I do not see additional value in polluting the completion. In real-life zsh would "stop" at them anyway, albeit with a trailing dot

Are you referring to the header like lint.pyupgrade, lint.ruff, etc. as is in my screenshot above? If so, we should include them even though they don't have documentation because ruff config lint.ruff will list out all the config options:
$ 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 lint.isort do not have any associated documentation, and ruff config just falls back to showing list of possible subkeys/fields. But with auto-completion, there is now no need to run ruff config lint.isort, pressing the tab gives even better output than running the command.

See, how the flow looks at the current commit. I first run ruff config lint.isort, and then do double-tab instead:

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 Lists all available config fields or subconfig keys - why would anyone care auto-completing to that name if they already have the full list with available on screen?

An important note - if lint.isort would get a doc of its own, it will appear in the list. So not all of the config groups are skipped.

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.

dhruvmanila · 2025-01-21T03:37:06Z

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 ruff config command and I don't think completion list should filter out certain values based on usages. This logic also seems to rely on the fact that the plugin headers doesn't have documentation but it might in the future which means that the suggestions would start including them. It might be that you and I don't use or find it useful but there might be valid use cases that we aren't seeing and users might find it confusing that a possible value is not being suggested. Does that make sense?

MichaReiser · 2025-01-21T07:55:57Z

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).

dhruvmanila · 2025-01-21T12:05:21Z

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, ruff config is mainly to display the documentation of the option or list down all available options for a plugin. Are you referring to a possible future extension of ruff config set?

crates/ruff_workspace/src/cli.rs

mishamsk · 2025-01-21T13:25:49Z

@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:

As @dhruvmanila suggested, show all valid values for the command. In that case, I suggest keeping doc for OptionSet's that have them AND adding "Show all available subconfig keys" for those that do not
As @MichaReiser suggest filter all OptionSet's from the result and keep only the fields

I'll follow your lead folks, let me know.

MichaReiser

@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

MichaReiser · 2025-01-23T07:32:24Z

crates/ruff_workspace/src/cli.rs

+                let first_paragraph = doc
+                    .lines()
+                    .take_while(|l| !l.trim_end().is_empty())
+                    .map(|s| s.replace('"', "'"))


Can you explain why replacing all " with ' is necessary? It does seem dangerous to me in the case the doc contains any code examples

@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.

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.

MichaReiser · 2025-01-23T07:34:06Z

crates/ruff_workspace/src/cli.rs

+                if let Some(arg) = arg {
+                    error.insert(
+                        clap::error::ContextKind::InvalidArg,
+                        clap::error::ContextValue::String(arg.to_string()),
+                    );
+                }


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

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.

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?

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

MichaReiser · 2025-01-23T07:35:43Z

crates/ruff_workspace/src/cli.rs

+        }
+    }
+
+    /// Opaque type for option strings on the command line.


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

mishamsk · 2025-01-24T02:44:38Z

@dhruvmanila convinced me. I think showing all values is a great starting point.

Made the change

Can you add an integration test similar to the tests we have

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

dhruvmanila · 2025-01-24T04:58:05Z

Unfortunately, looks like clap only generates proper completions for zsh, so this would not make any difference for bash/fish.

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.

dhruvmanila · 2025-01-24T05:07:14Z

crates/ruff/src/commands/completions/config.rs

+            group
+                .documentation()
+                .unwrap_or("Print a list of available options")
+                .to_owned(),


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

/// Configures how Ruff checks your code.

///

/// Options specified in the `lint` section take precedence over the deprecated top-level settings.

#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]

#[derive(Clone, Debug, PartialEq, Eq, Default, OptionsMetadata, Serialize, Deserialize)]

#[serde(

from = "LintOptionsWire",

deny_unknown_fields,

rename_all = "kebab-case"

)]

pub struct LintOptions {

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

/// Options for the `pydoclint` plugin.

#[option_group]

pub pydoclint: Option<PydoclintOptions>,

should be copied on top of PydoclintOptions which will then be used as the completion description.

crates/ruff/src/commands/completions/config.rs

dhruvmanila

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.

MichaReiser

This is great, thank you

mishamsk · 2025-01-25T20:45:04Z

@MichaReiser

removed default doc text per @dhruvmanila ask, I will open a follow-up and fill in the missing docs as soon as this one is merged
answered your question rgd error message
applied two nit's from @dhruvmanila

should be ok to merge

dhruvmanila

Thank you!

## Summary As promised in #15603 - the **highly** sophisticated change - adding missing config docstrings that are used in command completions. ## Test Plan I actually made a local change to emit all empty items and verified there are none now, before opening the PR.

* main: [red-knot] Extend instance-attribute tests (#15808) Fix formatter warning message for `flake8-quotes` option (#15788) [`flake8-bugbear`] Exempt `NewType` calls where the original type is immutable (`B008`) (#15765) Add missing config docstrings (#15803) [`refurb`] Do not emit diagnostic when loop variables are used outside loop body (`FURB122`) (#15757) [`ruff`] Check for shadowed `map` before suggesting fix (`RUF058`) (#15790) [red-knot] Do not use explicit `knot_extensions.Unknown` declaration (#15787) Preserve quotes in generated byte strings (#15778) [minor] Simplify some `ExprStringLiteral` creation logic (#15775) Preserve quote style in generated code (#15726) Rename internal helper functions (#15771) [`airflow`] Extend airflow context parameter check for `BaseOperator.execute` (`AIR302`) (#15713) Implement tab autocomplete for `ruff config` (#15603)

implement tab autocomplete for ruff config. fixes astral-sh#4551

5289aaa

dhruvmanila reviewed Jan 20, 2025

View reviewed changes

crates/ruff_workspace/src/cli.rs Outdated Show resolved Hide resolved

dhruvmanila added the cli Related to the command-line interface label Jan 20, 2025

mishamsk added 2 commits January 20, 2025 09:41

do not add doc-less config sets to completion

76b075e

emit the full first paragraph for completion doc

replace double quotes with sinlge in docs. fixes clap failing to esca…

38cdaa4

…pe them

dhruvmanila reviewed Jan 21, 2025

View reviewed changes

crates/ruff_workspace/src/cli.rs Outdated Show resolved Hide resolved

MichaReiser requested changes Jan 23, 2025

View reviewed changes

move completions to a new mod in ruff crate

630d622

show all option sets, even without a doc do not show completion values in help

mishamsk requested a review from MichaReiser January 24, 2025 03:20

dhruvmanila reviewed Jan 24, 2025

View reviewed changes

crates/ruff/src/commands/completions/config.rs Outdated Show resolved Hide resolved

dhruvmanila reviewed Jan 24, 2025

View reviewed changes

crates/ruff/src/commands/completions/config.rs Outdated Show resolved Hide resolved

dhruvmanila approved these changes Jan 24, 2025

View reviewed changes

MichaReiser approved these changes Jan 24, 2025

View reviewed changes

do not supply default value for empty docs

312504a

dhruvmanila approved these changes Jan 27, 2025

View reviewed changes

dhruvmanila merged commit 646f194 into astral-sh:main Jan 27, 2025
21 checks passed

mishamsk deleted the 4551-add-shell-completions-for-config branch January 27, 2025 15:33

mishamsk mentioned this pull request Jan 29, 2025

Add missing config docstrings #15803

Merged

BrewTestBot mentioned this pull request Jan 30, 2025

ruff 0.9.4 Homebrew/homebrew-core#206058

Merged

	/// Configures how Ruff checks your code.
	///
	/// Options specified in the `lint` section take precedence over the deprecated top-level settings.
	#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
	#[derive(Clone, Debug, PartialEq, Eq, Default, OptionsMetadata, Serialize, Deserialize)]
	#[serde(
	from = "LintOptionsWire",
	deny_unknown_fields,
	rename_all = "kebab-case"
	)]
	pub struct LintOptions {

	/// Options for the `pydoclint` plugin.
	#[option_group]
	pub pydoclint: Option<PydoclintOptions>,

Comments

Conversation

mishamsk commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mishamsk commented Jan 20, 2025

Uh oh!

dhruvmanila commented Jan 20, 2025

Uh oh!

mishamsk commented Jan 20, 2025

Uh oh!

dhruvmanila commented Jan 21, 2025

Uh oh!

MichaReiser commented Jan 21, 2025

Uh oh!

dhruvmanila commented Jan 21, 2025

Uh oh!

Uh oh!

mishamsk commented Jan 21, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mishamsk Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mishamsk commented Jan 24, 2025

Uh oh!

dhruvmanila commented Jan 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dhruvmanila left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

mishamsk commented Jan 25, 2025

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mishamsk commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

`ruff-ecosystem` results

mishamsk Jan 24, 2025 •

edited

Loading

dhruvmanila left a comment •

edited

Loading