Skip to content
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

Better error message when --config is given a table key and a non-inline-table value #15266

Merged
merged 7 commits into from
Jan 6, 2025

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #13995.

Test Plan

cargo nextest run and cargo insta test.

Copy link
Contributor

github-actions bot commented Jan 5, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood self-requested a review January 5, 2025 11:15
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 5, 2025

This is great! I think we can do even better with a few tweaks, though. With your branch currently, this is the error message I get:

~/dev/ruff (cli-config)⚡ % cargo run -- check . --config='lint.flake8-pytest-style="csv"'
   Compiling ruff v0.8.6 (/Users/alexw/dev/ruff/crates/ruff)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.51s
     Running `target/debug/ruff check . '--config=lint.flake8-pytest-style="csv"'`
error: invalid value 'lint.flake8-pytest-style="csv"' for '--config <CONFIG_OPTION>'

  tip: A `--config` flag must either be a path to a `.toml` configuration file
       or a TOML `<KEY> = <VALUE>` pair overriding a specific configuration
       option

`lint.flake8-pytest-style` is a table.
Did you mean to use one of its subkeys instead? Possible choices:
lint.flake8-pytest-style.fixture-parentheses
lint.flake8-pytest-style.parametrize-names-type
lint.flake8-pytest-style.parametrize-values-type
lint.flake8-pytest-style.parametrize-values-row-type
lint.flake8-pytest-style.raises-require-match-for
lint.flake8-pytest-style.raises-extend-require-match-for
lint.flake8-pytest-style.mark-parentheses

Could not parse the supplied argument as a `ruff.toml` configuration option:

invalid type: string "csv", expected struct Flake8PytestStyleOptions
in `lint`

For more information, try '--help'.
Colorized screenshot

image

But if I apply this diff to your branch:

--- a/crates/ruff/src/args.rs
+++ b/crates/ruff/src/args.rs
@@ -978,23 +978,25 @@ The path `{value}` does not point to a configuration file"
                     let prefixed_subkeys = format!("{set}")
                         .trim_ascii()
                         .split('\n')
-                        .map(|child| format!("{key}.{child}"))
+                        .map(|child| format!(" - `{key}.{child}`"))
                         .join("\n");
 
                     tip.push_str(&format!(
                         "
 
-`{key}` is a table.
-Did you mean to use one of its subkeys instead? Possible choices:
+`{key}` is a table of configuration options, but only a single option can be
+overridden with `--config`. Did you want to override one of the table's subkeys?
+
+Possible choices:
 {prefixed_subkeys}"
                     ));
                 }
-            };
-
-            tip.push_str(&format!(
-                "\n\n{}:\n\n{underlying_error}",
-                config_parse_error.description()
-            ));
+            } else {
+                tip.push_str(&format!(
+                    "\n\n{}:\n\n{underlying_error}",
+                    config_parse_error.description()
+                ));
+            }
         }

Then it becomes:

~/dev/ruff (cli-config)⚡ [2] % cargo run -- check . --config='lint.flake8-pytest-style="csv"'
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.36s
     Running `target/debug/ruff check . '--config=lint.flake8-pytest-style="csv"'`
error: invalid value 'lint.flake8-pytest-style="csv"' for '--config <CONFIG_OPTION>'

  tip: A `--config` flag must either be a path to a `.toml` configuration file
       or a TOML `<KEY> = <VALUE>` pair overriding a specific configuration
       option

`lint.flake8-pytest-style` is a table of configuration options, but only a single option can be
overridden with `--config`. Did you want to override one of the table's subkeys?

Possible choices:
 - `lint.flake8-pytest-style.fixture-parentheses`
 - `lint.flake8-pytest-style.parametrize-names-type`
 - `lint.flake8-pytest-style.parametrize-values-type`
 - `lint.flake8-pytest-style.parametrize-values-row-type`
 - `lint.flake8-pytest-style.raises-require-match-for`
 - `lint.flake8-pytest-style.raises-extend-require-match-for`
 - `lint.flake8-pytest-style.mark-parentheses`

For more information, try '--help'.
Colorized screenshot

image

which feels significantly more readable to me. WDYT?

@InSyncWithFoo
Copy link
Contributor Author

InSyncWithFoo commented Jan 5, 2025

@AlexWaygood I was actually aiming for copyability, but readability is fine too.

[...] but only a single option can be overridden with --config.

This is rather misleading, because you can pass an inline table as the value:

# Same as "lint.select = ['I001']"
$ ruff check --isolated --config="lint = { select = ['I001'] }"

@AlexWaygood
Copy link
Member

This is rather misleading, because you can pass an inline table as the value:

Oh, great point! I forgot how my own feature worked :-) We can scrap that clause, then.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@MichaReiser MichaReiser added the cli Related to the command-line interface label Jan 6, 2025
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I'm a bit worried about the output parsing and I think we can improve the handling if set itself has sub-table entries.

@MichaReiser MichaReiser enabled auto-merge (squash) January 6, 2025 13:11
crates/ruff/tests/lint.rs Outdated Show resolved Hide resolved
crates/ruff/tests/lint.rs Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood disabled auto-merge January 6, 2025 13:14
@AlexWaygood AlexWaygood enabled auto-merge (squash) January 6, 2025 13:15
@AlexWaygood AlexWaygood merged commit 832c0fa into astral-sh:main Jan 6, 2025
20 checks passed
@InSyncWithFoo InSyncWithFoo deleted the cli-config branch January 6, 2025 13:23
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (60 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
dcreager added a commit that referenced this pull request Jan 6, 2025
* main: (29 commits)
  [`ruff`] Dataclass enums (`RUF049`) (#15299)
  Better error message when `--config` is given a table key and a non-inline-table value (#15266)
  Update pre-commit dependencies (#15289)
  Don't fix in ecosystem check (#15267)
  Update Rust crate itertools to 0.14.0 (#15287)
  Remove accidental empty block at the bottom of `split-static-string (SIM905)` doc (#15290)
  Update Rust crate clearscreen to v4 (#15288)
  Update Rust crate insta to v1.42.0 (#15286)
  Update NPM Development dependencies (#15285)
  Update dependency uuid to v11.0.4 (#15284)
  Update dependency ruff to v0.8.6 (#15283)
  Update Rust crate syn to v2.0.95 (#15282)
  Update Rust crate matchit to v0.8.6 (#15281)
  Update Rust crate bstr to v1.11.3 (#15280)
  [red-knot] Future-proof `Type::is_disjoint_from()` (#15262)
  [red-knot] Improve `Type::is_disjoint_from()` for `KnownInstanceType`s (#15261)
  [red-knot] Minor simplifications and improvements to constraint narrowing logic (#15270)
  Allow assigning ellipsis literal as parameter default value (#14982)
  [red-knot] fix control flow for assignment expressions in elif tests (#15274)
  [`refurb`] Mark fix as unsafe when the right-hand side is a string (`FURB171`) (#15273)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set config option with struct type from command-line
3 participants