Skip to content

Commit

Permalink
Better error message when --config is given a table key and a non-i…
Browse files Browse the repository at this point in the history
…nline-table value (#15266)

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
  • Loading branch information
3 people authored Jan 6, 2025
1 parent f29c9e4 commit 832c0fa
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 5 deletions.
37 changes: 32 additions & 5 deletions crates/ruff/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use anyhow::bail;
use clap::builder::{TypedValueParser, ValueParserFactory};
use clap::{command, Parser, Subcommand};
use colored::Colorize;
use itertools::Itertools;
use path_absolutize::path_dedot;
use regex::Regex;
use ruff_graph::Direction;
Expand All @@ -24,6 +25,7 @@ use ruff_source_file::{LineIndex, OneIndexed};
use ruff_text_size::TextRange;
use ruff_workspace::configuration::{Configuration, RuleSelection};
use ruff_workspace::options::{Options, PycodestyleOptions};
use ruff_workspace::options_base::{OptionEntry, OptionsMetadata};
use ruff_workspace::resolver::ConfigurationTransformer;
use rustc_hash::FxHashMap;
use toml;
Expand Down Expand Up @@ -967,11 +969,36 @@ It looks like you were trying to pass a path to a configuration file.
The path `{value}` does not point to a configuration file"
));
}
} else if value.contains('=') {
tip.push_str(&format!(
"\n\n{}:\n\n{underlying_error}",
config_parse_error.description()
));
} else if let Some((key, value)) = value.split_once('=') {
let key = key.trim_ascii();
let value = value.trim_ascii_start();

match Options::metadata().find(key) {
Some(OptionEntry::Set(set)) if !value.starts_with('{') => {
let prefixed_subfields = set
.collect_fields()
.iter()
.map(|(name, _)| format!("- `{key}.{name}`"))
.join("\n");

tip.push_str(&format!(
"
`{key}` is a table of configuration options.
Did you want to override one of the table's subkeys?
Possible choices:
{prefixed_subfields}"
));
}
_ => {
tip.push_str(&format!(
"\n\n{}:\n\n{underlying_error}",
config_parse_error.description()
));
}
}
}
let tip = tip.trim_end().to_owned().into();

Expand Down
84 changes: 84 additions & 0 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,90 @@ fn each_toml_option_requires_a_new_flag_2() {
");
}

#[test]
fn value_given_to_table_key_is_not_inline_table_1() {
// https://github.com/astral-sh/ruff/issues/13995
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args([".", "--config", r#"lint.flake8-pytest-style="csv""#]),
@r#"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
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.
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'.
"#);
}

#[test]
fn value_given_to_table_key_is_not_inline_table_2() {
// https://github.com/astral-sh/ruff/issues/13995
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args([".", "--config", r#"lint=123"#]),
@r"
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: invalid value 'lint=123' 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` is a table of configuration options.
Did you want to override one of the table's subkeys?
Possible choices:
- `lint.allowed-confusables`
- `lint.dummy-variable-rgx`
- `lint.extend-ignore`
- `lint.extend-select`
- `lint.extend-fixable`
- `lint.external`
- `lint.fixable`
- `lint.ignore`
- `lint.extend-safe-fixes`
- `lint.extend-unsafe-fixes`
- `lint.ignore-init-module-imports`
- `lint.logger-objects`
- `lint.select`
- `lint.explicit-preview-rules`
- `lint.task-tags`
- `lint.typing-modules`
- `lint.unfixable`
- `lint.per-file-ignores`
- `lint.extend-per-file-ignores`
- `lint.exclude`
- `lint.preview`
For more information, try '--help'.
");
}

#[test]
fn config_doubly_overridden_via_cli() -> Result<()> {
let tempdir = TempDir::new()?;
Expand Down
16 changes: 16 additions & 0 deletions crates/ruff_workspace/src/options_base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,22 @@ impl OptionSet {
None
}
}

pub fn collect_fields(&self) -> Vec<(String, OptionField)> {
struct FieldsCollector(Vec<(String, OptionField)>);

impl Visit for FieldsCollector {
fn record_field(&mut self, name: &str, field: OptionField) {
self.0.push((name.to_string(), field));
}

fn record_set(&mut self, _name: &str, _group: OptionSet) {}
}

let mut visitor = FieldsCollector(vec![]);
self.record(&mut visitor);
visitor.0
}
}

/// Visitor that writes out the names of all fields and sets.
Expand Down

0 comments on commit 832c0fa

Please sign in to comment.