From ec14d0820bde5a46c854b079aeed0d67ac6c036a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 4 Dec 2024 16:14:38 +0900 Subject: [PATCH] cli: replace uses of .get_table() to include better error indication This will also help migrate to toml_edit, where a value type doesn't provide a deserialization helper. --- cli/src/cli_util.rs | 41 ++++++++++-------------- cli/src/commands/fix.rs | 60 +++++++++++++++++------------------ cli/src/complete.rs | 3 +- cli/src/formatter.rs | 4 +-- cli/tests/test_alias.rs | 18 +++++++---- cli/tests/test_fix_command.rs | 16 ++++++---- 6 files changed, 71 insertions(+), 71 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index fa31cff8e6..13f05f8bb0 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -3148,22 +3148,20 @@ fn resolve_aliases( app: &Command, mut string_args: Vec, ) -> Result, CommandError> { - let mut aliases_map = config.get_table("aliases")?; + let defined_aliases: HashSet<_> = config.table_keys("aliases").collect(); let mut resolved_aliases = HashSet::new(); let mut real_commands = HashSet::new(); for command in app.get_subcommands() { - real_commands.insert(command.get_name().to_string()); + real_commands.insert(command.get_name()); for alias in command.get_all_aliases() { - real_commands.insert(alias.to_string()); + real_commands.insert(alias); } } - for alias in aliases_map.keys() { - if real_commands.contains(alias) { - writeln!( - ui.warning_default(), - "Cannot define an alias that overrides the built-in command '{alias}'" - )?; - } + for alias in defined_aliases.intersection(&real_commands).sorted() { + writeln!( + ui.warning_default(), + "Cannot define an alias that overrides the built-in command '{alias}'" + )?; } loop { @@ -3177,24 +3175,19 @@ fn resolve_aliases( .unwrap_or_default() .map(|arg| arg.to_str().unwrap().to_string()) .collect_vec(); - if resolved_aliases.contains(&alias_name) { + if resolved_aliases.contains(&*alias_name) { return Err(user_error(format!( r#"Recursive alias definition involving "{alias_name}""# ))); } - if let Some(value) = aliases_map.remove(&alias_name) { - if let Ok(alias_definition) = value.try_deserialize::>() { - assert!(string_args.ends_with(&alias_args)); - string_args.truncate(string_args.len() - 1 - alias_args.len()); - string_args.extend(alias_definition); - string_args.extend_from_slice(&alias_args); - resolved_aliases.insert(alias_name.clone()); - continue; - } else { - return Err(user_error(format!( - r#"Alias definition for "{alias_name}" must be a string list"# - ))); - } + if let Some(&alias_name) = defined_aliases.get(&*alias_name) { + let alias_definition: Vec = config.get(["aliases", alias_name])?; + assert!(string_args.ends_with(&alias_args)); + string_args.truncate(string_args.len() - 1 - alias_args.len()); + string_args.extend(alias_definition); + string_args.extend_from_slice(&alias_args); + resolved_aliases.insert(alias_name); + continue; } else { // Not a real command and not an alias, so return what we've resolved so far return Ok(string_args); diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 6a6508bfbe..33a7f96968 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -473,38 +473,36 @@ fn get_tools_config(ui: &mut Ui, settings: &UserSettings) -> Result = tools_table - .into_iter() - .sorted_by(|a, b| a.0.cmp(&b.0)) - .map(|(name, value)| -> Result { - let mut diagnostics = FilesetDiagnostics::new(); - let tool: RawToolConfig = value.try_deserialize()?; - let expression = FilesetExpression::union_all( - tool.patterns - .iter() - .map(|arg| { - fileset::parse( - &mut diagnostics, - arg, - &RepoPathUiConverter::Fs { - cwd: "".into(), - base: "".into(), - }, - ) - }) - .try_collect()?, - ); - print_parse_diagnostics(ui, &format!("In `fix.tools.{name}`"), &diagnostics)?; - Ok(ToolConfig { - command: tool.command, - matcher: expression.to_matcher(), - }) + let tools: Vec = settings + .table_keys("fix.tools") + // Sort keys early so errors are deterministic. + .sorted() + .map(|name| -> Result { + let mut diagnostics = FilesetDiagnostics::new(); + let tool: RawToolConfig = settings.get(["fix", "tools", name])?; + let expression = FilesetExpression::union_all( + tool.patterns + .iter() + .map(|arg| { + fileset::parse( + &mut diagnostics, + arg, + &RepoPathUiConverter::Fs { + cwd: "".into(), + base: "".into(), + }, + ) + }) + .try_collect()?, + ); + print_parse_diagnostics(ui, &format!("In `fix.tools.{name}`"), &diagnostics)?; + Ok(ToolConfig { + command: tool.command, + matcher: expression.to_matcher(), }) - .try_collect()?; - tools_config.tools.append(&mut tools); - } + }) + .try_collect()?; + tools_config.tools.extend(tools); if tools_config.tools.is_empty() { // TODO: This is not a useful message when one or both fields are present but // have the wrong type. After removing `fix.tool-command`, it will be simpler to diff --git a/cli/src/complete.rs b/cli/src/complete.rs index a79c9ed6ee..c3404499ca 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -206,8 +206,7 @@ pub fn git_remotes() -> Vec { pub fn aliases() -> Vec { with_jj(|_, settings| { Ok(settings - .get_table("aliases")? - .into_keys() + .table_keys("aliases") // This is opinionated, but many people probably have several // single- or two-letter aliases they use all the time. These // aliases don't need to be completed and they would only clutter diff --git a/cli/src/formatter.rs b/cli/src/formatter.rs index d2d15c24dd..b8bb03a8b8 100644 --- a/cli/src/formatter.rs +++ b/cli/src/formatter.rs @@ -407,8 +407,7 @@ impl ColorFormatter { fn rules_from_config(config: &StackedConfig) -> Result { let mut result = vec![]; - let table = config.get_table("colors")?; - for (key, value) in table { + for key in config.table_keys("colors") { let to_config_err = |message: String| ConfigGetError::Type { name: format!("colors.'{key}'"), error: message.into(), @@ -418,6 +417,7 @@ fn rules_from_config(config: &StackedConfig) -> Result { .split_whitespace() .map(ToString::to_string) .collect_vec(); + let value = config.get_value(["colors", key])?; match value.kind { config::ValueKind::String(color_name) => { let style = Style { diff --git a/cli/tests/test_alias.rs b/cli/tests/test_alias.rs index f0aa5f9281..916e5596aa 100644 --- a/cli/tests/test_alias.rs +++ b/cli/tests/test_alias.rs @@ -234,13 +234,19 @@ fn test_alias_invalid_definition() { "#, ); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-list"]); - insta::assert_snapshot!(stderr, @r###" - Error: Alias definition for "non-list" must be a string list - "###); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r" + Config error: Invalid type or value for aliases.non-list + Caused by: invalid type: integer `5`, expected a sequence + Hint: Check the config file: $TEST_ENV/config/config0002.toml + For help, see https://martinvonz.github.io/jj/latest/config/. + "); let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["non-string-list"]); - insta::assert_snapshot!(stderr, @r###" - Error: Alias definition for "non-string-list" must be a string list - "###); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r" + Config error: Invalid type or value for aliases.non-string-list + Caused by: invalid type: sequence, expected a string for key `[0]` in config/config0002.toml + Hint: Check the config file: $TEST_ENV/config/config0002.toml + For help, see https://martinvonz.github.io/jj/latest/config/. + "); } #[test] diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 8a963d4f65..734d77459b 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -256,10 +256,12 @@ fn test_config_tables_all_commands_missing() { std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); - insta::assert_snapshot!(stderr, @r###" - Config error: missing field `command` + insta::assert_snapshot!(stderr.replace('\\', "/"), @r" + Config error: Invalid type or value for fix.tools.my-tool-missing-command-1 + Caused by: missing field `command` + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); insta::assert_snapshot!(content, @"foo\n"); @@ -288,10 +290,12 @@ fn test_config_tables_some_commands_missing() { std::fs::write(repo_path.join("foo"), "foo\n").unwrap(); let stderr = test_env.jj_cmd_failure(&repo_path, &["fix"]); - insta::assert_snapshot!(stderr, @r###" - Config error: missing field `command` + insta::assert_snapshot!(stderr.replace('\\', "/"), @r" + Config error: Invalid type or value for fix.tools.my-tool-missing-command + Caused by: missing field `command` + Hint: Check the config file: $TEST_ENV/config/config0002.toml For help, see https://martinvonz.github.io/jj/latest/config/. - "###); + "); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "foo", "-r", "@"]); insta::assert_snapshot!(content, @"foo\n");