From ecd64aae8e4a8ea630047164225e021bc029a324 Mon Sep 17 00:00:00 2001 From: Remo Senekowitsch Date: Sat, 16 Nov 2024 18:42:26 +0100 Subject: [PATCH] completion: teach config about keys There are two known limitations right now: - Only statically known keys are suggested. - Keys that the user did not set are still suggested for `jj config get`. Running that suggestion may result in an error. The error message will be appropriate though and there is some value in letting the user know that this config value theoretically exists. Some users may try to explore what configurations are available via the completions. --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/src/commands/config/get.rs | 4 +- cli/src/commands/config/list.rs | 3 ++ cli/src/commands/config/set.rs | 4 +- cli/src/commands/config/unset.rs | 4 +- cli/src/commands/util/config_schema.rs | 5 +- cli/src/complete.rs | 73 ++++++++++++++++++++++++++ cli/src/config.rs | 3 ++ cli/tests/test_completion.rs | 22 ++++++++ 10 files changed, 114 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e0bccea695..2490ba3939 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1845,6 +1845,7 @@ dependencies = [ "sapling-renderdag", "scm-record", "serde", + "serde_json", "slab", "strsim", "tempfile", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 4ec287923f..57be178226 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -80,6 +80,7 @@ rpassword = { workspace = true } sapling-renderdag = { workspace = true } scm-record = { workspace = true } serde = { workspace = true } +serde_json = { workspace = true } slab = { workspace = true } strsim = { workspace = true } tempfile = { workspace = true } diff --git a/cli/src/commands/config/get.rs b/cli/src/commands/config/get.rs index abeb4eebd2..70184cf78e 100644 --- a/cli/src/commands/config/get.rs +++ b/cli/src/commands/config/get.rs @@ -14,11 +14,13 @@ use std::io::Write as _; +use clap_complete::ArgValueCandidates; use tracing::instrument; use crate::cli_util::CommandHelper; use crate::command_error::config_error; use crate::command_error::CommandError; +use crate::complete; use crate::config::ConfigNamePathBuf; use crate::ui::Ui; @@ -34,7 +36,7 @@ use crate::ui::Ui; #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub struct ConfigGetArgs { - #[arg(required = true)] + #[arg(required = true, add = ArgValueCandidates::new(complete::leaf_config_keys))] name: ConfigNamePathBuf, } diff --git a/cli/src/commands/config/list.rs b/cli/src/commands/config/list.rs index 8308171af9..e4501a40fb 100644 --- a/cli/src/commands/config/list.rs +++ b/cli/src/commands/config/list.rs @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use tracing::instrument; use super::ConfigLevelArgs; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; +use crate::complete; use crate::config::to_toml_value; use crate::config::AnnotatedValue; use crate::config::ConfigNamePathBuf; @@ -31,6 +33,7 @@ use crate::ui::Ui; #[command(mut_group("config_level", |g| g.required(false)))] pub struct ConfigListArgs { /// An optional name of a specific config option to look up. + #[arg(add = ArgValueCandidates::new(complete::config_keys))] pub name: Option, /// Whether to explicitly include built-in default values in the list. #[arg(long, conflicts_with = "config_level")] diff --git a/cli/src/commands/config/set.rs b/cli/src/commands/config/set.rs index e4a9106edc..ff4f816c5f 100644 --- a/cli/src/commands/config/set.rs +++ b/cli/src/commands/config/set.rs @@ -14,6 +14,7 @@ use std::io; +use clap_complete::ArgValueCandidates; use jj_lib::commit::Commit; use jj_lib::repo::Repo; use tracing::instrument; @@ -24,6 +25,7 @@ use crate::cli_util::CommandHelper; use crate::cli_util::WorkspaceCommandHelper; use crate::command_error::user_error; use crate::command_error::CommandError; +use crate::complete; use crate::config::parse_toml_value_or_bare_string; use crate::config::write_config_value_to_file; use crate::config::ConfigNamePathBuf; @@ -32,7 +34,7 @@ use crate::ui::Ui; /// Update config file to set the given option to a given value. #[derive(clap::Args, Clone, Debug)] pub struct ConfigSetArgs { - #[arg(required = true)] + #[arg(required = true, add = ArgValueCandidates::new(complete::leaf_config_keys))] name: ConfigNamePathBuf, #[arg(required = true)] value: String, diff --git a/cli/src/commands/config/unset.rs b/cli/src/commands/config/unset.rs index 46a8446f39..1b8400c7af 100644 --- a/cli/src/commands/config/unset.rs +++ b/cli/src/commands/config/unset.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use clap_complete::ArgValueCandidates; use tracing::instrument; use super::ConfigLevelArgs; @@ -19,6 +20,7 @@ use crate::cli_util::get_new_config_file_path; use crate::cli_util::CommandHelper; use crate::command_error::user_error; use crate::command_error::CommandError; +use crate::complete; use crate::config::remove_config_value_from_file; use crate::config::ConfigNamePathBuf; use crate::ui::Ui; @@ -26,7 +28,7 @@ use crate::ui::Ui; /// Update config file to unset the given option. #[derive(clap::Args, Clone, Debug)] pub struct ConfigUnsetArgs { - #[arg(required = true)] + #[arg(required = true, add = ArgValueCandidates::new(complete::leaf_config_keys))] name: ConfigNamePathBuf, #[command(flatten)] level: ConfigLevelArgs, diff --git a/cli/src/commands/util/config_schema.rs b/cli/src/commands/util/config_schema.rs index 7923dc02f6..91e464a5e4 100644 --- a/cli/src/commands/util/config_schema.rs +++ b/cli/src/commands/util/config_schema.rs @@ -16,6 +16,7 @@ use std::io::Write as _; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; +use crate::config::CONFIG_SCHEMA; use crate::ui::Ui; /// Print the JSON schema for the jj TOML config format. @@ -27,8 +28,6 @@ pub fn cmd_util_config_schema( _command: &CommandHelper, _args: &UtilConfigSchemaArgs, ) -> Result<(), CommandError> { - // TODO(#879): Consider generating entire schema dynamically vs. static file. - let buf = include_bytes!("../../config-schema.json"); - ui.stdout().write_all(buf)?; + ui.stdout().write_all(CONFIG_SCHEMA.as_bytes())?; Ok(()) } diff --git a/cli/src/complete.rs b/cli/src/complete.rs index d138dc4327..6418849afb 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -26,7 +26,9 @@ use crate::cli_util::GlobalArgs; use crate::command_error::user_error; use crate::command_error::CommandError; use crate::config::default_config; +use crate::config::ConfigNamePathBuf; use crate::config::LayeredConfigs; +use crate::config::CONFIG_SCHEMA; use crate::ui::Ui; const BOOKMARK_HELP_TEMPLATE: &str = r#" @@ -292,6 +294,66 @@ pub fn workspaces() -> Vec { }) } +fn config_keys_rec( + prefix: ConfigNamePathBuf, + properties: &serde_json::Map, + acc: &mut Vec, + only_leaves: bool, +) { + for (key, value) in properties { + let mut prefix = prefix.clone(); + prefix.push(key); + + let value = value.as_object().unwrap(); + match value.get("type").and_then(|v| v.as_str()) { + Some("object") => { + if !only_leaves { + let help = value + .get("description") + .map(|desc| desc.as_str().unwrap().to_string().into()); + let escaped_key = prefix.to_string(); + acc.push(CompletionCandidate::new(escaped_key).help(help)); + } + let Some(properties) = value.get("properties") else { + continue; + }; + let properties = properties.as_object().unwrap(); + config_keys_rec(prefix, properties, acc, only_leaves); + } + _ => { + let help = value + .get("description") + .map(|desc| desc.as_str().unwrap().to_string().into()); + let escaped_key = prefix.to_string(); + acc.push(CompletionCandidate::new(escaped_key).help(help)); + } + } + } +} + +fn config_keys_impl(only_leaves: bool) -> Vec { + let schema: serde_json::Value = serde_json::from_str(CONFIG_SCHEMA).unwrap(); + let schema = schema.as_object().unwrap(); + let properties = schema["properties"].as_object().unwrap(); + + let mut candidates = Vec::new(); + config_keys_rec( + ConfigNamePathBuf::root(), + properties, + &mut candidates, + only_leaves, + ); + candidates +} + +pub fn config_keys() -> Vec { + config_keys_impl(false) +} + +pub fn leaf_config_keys() -> Vec { + config_keys_impl(true) +} + /// Shell out to jj during dynamic completion generation /// /// In case of errors, print them and early return an empty vector. @@ -402,3 +464,14 @@ fn get_jj_command() -> Result<(std::process::Command, Config), CommandError> { Ok((cmd, config)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_config_keys() { + // Just make sure the schema is parsed without failure. + let _ = config_keys(); + } +} diff --git a/cli/src/config.rs b/cli/src/config.rs index 9f424567be..71b5775ba6 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -35,6 +35,9 @@ use crate::command_error::user_error; use crate::command_error::user_error_with_message; use crate::command_error::CommandError; +// TODO(#879): Consider generating entire schema dynamically vs. static file. +pub const CONFIG_SCHEMA: &str = include_str!("config-schema.json"); + /// Parses a TOML value expression. Interprets the given value as string if it /// can't be parsed. pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value { diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index 3bd1fff788..d7d1031c02 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -472,3 +472,25 @@ fn test_workspaces() { default initial "); } + +#[test] +fn test_config() { + let mut test_env = TestEnvironment::default(); + test_env.add_env_var("COMPLETE", "fish"); + let dir = test_env.env_root(); + + let stdout = test_env.jj_cmd_success(dir, &["--", "jj", "config", "get", "c"]); + insta::assert_snapshot!(stdout, @r" + core.fsmonitor Whether to use an external filesystem monitor, useful for large repos + core.watchman.register_snapshot_trigger Whether to use triggers to monitor for changes in the background. + "); + + let stdout = test_env.jj_cmd_success(dir, &["--", "jj", "config", "list", "c"]); + insta::assert_snapshot!(stdout, @r" + colors Mapping from jj formatter labels to colors + core + core.fsmonitor Whether to use an external filesystem monitor, useful for large repos + core.watchman + core.watchman.register_snapshot_trigger Whether to use triggers to monitor for changes in the background. + "); +}