From 80e9abe2a05416beca8e6784b24e7d997e47bfe2 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Mon, 4 Sep 2023 17:10:23 +0200 Subject: [PATCH] Handle `override` and `help` inputs in customize mode (#1563) * customize mode conflicts: improve help messages * Enablze overriding input fields with --override * Fix unescaped [ in rustdoc --- cli/src/export.rs | 159 +++++++++++++----- .../inputs/customize-mode/has_help.ncl | 8 + .../inputs/customize-mode/has_override.ncl | 8 + .../{freeform => customize-mode}/help.ncl | 0 .../not_an_input.ncl | 0 .../simple_adder.ncl | 0 .../unknown_override.ncl | 0 .../snapshot__export_stdout_has_help.ncl.snap | 28 +++ ...pshot__export_stdout_has_override.ncl.snap | 28 +++ .../snapshot__export_stdout_help.ncl.snap | 4 + 10 files changed, 193 insertions(+), 42 deletions(-) create mode 100644 cli/tests/snapshot/inputs/customize-mode/has_help.ncl create mode 100644 cli/tests/snapshot/inputs/customize-mode/has_override.ncl rename cli/tests/snapshot/inputs/{freeform => customize-mode}/help.ncl (100%) rename cli/tests/snapshot/inputs/{freeform => customize-mode}/not_an_input.ncl (100%) rename cli/tests/snapshot/inputs/{freeform => customize-mode}/simple_adder.ncl (100%) rename cli/tests/snapshot/inputs/{freeform => customize-mode}/unknown_override.ncl (100%) create mode 100644 cli/tests/snapshot/snapshots/snapshot__export_stdout_has_help.ncl.snap create mode 100644 cli/tests/snapshot/snapshots/snapshot__export_stdout_has_override.ncl.snap diff --git a/cli/src/export.rs b/cli/src/export.rs index b329f44814..4c13af5c53 100644 --- a/cli/src/export.rs +++ b/cli/src/export.rs @@ -36,6 +36,9 @@ const OVERRIDES_LIST_MAX_COUNT: usize = 15; /// a value. const NICKEL_VALUE_NAME: &str = "NICKEL EXPRESSION"; +const EXPERIMENTAL_MSG: &str = + "[WARNING] Customize mode is experimental. Its interface is subject to breaking changes."; + #[derive(clap::Parser, Debug)] pub struct ExportCommand { #[arg(long, value_enum, default_value_t)] @@ -48,10 +51,11 @@ pub struct ExportCommand { #[command(flatten)] pub evaluation: EvalCommand, - /// Enters customize mode. This turns the nickel invocation into a new CLI based on the - /// configuration to be exported, where the value of fields can be set directly through CLI - /// arguments. Print the corresponding help (`nickel export -f -- --help`) to see - /// available options. + /// \[WARNING\] Customize mode is experimental. Its interface is subject to breaking changes. + /// + /// Customize mode turns the nickel invocation into a new CLI based on the configuration to be + /// exported, where the value of fields can be set directly through CLI arguments. Print the + /// corresponding help (`nickel export -f -- --help`) to see available options. #[arg(last = true)] pub customize_mode: Vec, } @@ -114,32 +118,37 @@ impl TermInterface { /// In addition to the updated command, `build_cmd` returns a mapping from clap argument ids to /// their corresponding full field path as an array of fields. fn build_cmd(&self) -> TermCommand { - let mut paths = HashMap::new(); - let mut ovd_interfaces = BTreeMap::new(); + let clap_cmd = Command::new("customize-mode") + .about("Customize a Nickel configuration through the command line before exporting") + .after_help(EXPERIMENTAL_MSG) + .no_binary_name(true); - let mut cmd = Command::new("customize-mode").no_binary_name(true); + let mut term_cmd = TermCommand::new(clap_cmd); for (id, field) in &self.fields { - cmd = field.add_args(cmd, vec![id.to_string()], &mut paths, &mut ovd_interfaces) + term_cmd = field.add_args(term_cmd, vec![id.to_string()]) } - let mut overrides_list: Vec = ovd_interfaces + let mut overrides_list: Vec = term_cmd + .overrides .keys() .take(OVERRIDES_LIST_MAX_COUNT) .map(|field_path| format!("- {field_path}")) .collect(); - if ovd_interfaces.len() > OVERRIDES_LIST_MAX_COUNT { + if term_cmd.overrides.len() > OVERRIDES_LIST_MAX_COUNT { overrides_list.push("- ...".into()); } - //TODO: what happens if there's a field named override? + let has_override = term_cmd.inputs.contains_key("override"); + let has_help = term_cmd.inputs.contains_key("help"); + let override_arg_label = "override"; let override_help = format!( "Override any field of the configuration with a valid Nickel expression provided as \ a string. The new value will be merged with the configuration with a `force` \ - priority.\ - \n\nOverridable fields:\n{}\n", + priority.\n\n\ + Overridable fields:\n{}\n\n", overrides_list.join("\n") ); let override_arg = clap::Arg::new(override_arg_label) @@ -151,13 +160,35 @@ impl TermInterface { .required(false) .help(override_help); - cmd = cmd.arg(override_arg); + term_cmd.clap_cmd = term_cmd.clap_cmd.arg(override_arg); - TermCommand { - clap_cmd: cmd, - args: paths, - overrides: ovd_interfaces, + if has_help || has_override { + let conflict_field = if has_override { + "override" + } else if has_help { + "help" + } else { + // We tested in the parent `if` that one of those two booleans must be set + unreachable!() + }; + + let extra = if has_help && has_override { + ". The same applies to the conflicting `help` field" + } else { + "" + }; + + term_cmd.clap_cmd = term_cmd.clap_cmd.after_long_help(format!( + "\ + [CONFLICT] This configuration has a field named `{conflict_field}` which conflicts \ + with the built-in `--{conflict_field}` argument. To set this field to e.g. \"some_value\", \ + use `--override {conflict_field} \"some_value\"` instead of `--{conflict_field} \"some_value\"`\ + {extra}\n\n\ + {EXPERIMENTAL_MSG}" + )); } + + term_cmd } } @@ -314,39 +345,32 @@ impl FieldInterface { /// /// `add_args` updates `paths` as well, which maps clap argument ids to the corresponding field /// path represented as an array of string names. - fn add_args( - &self, - mut cmd: clap::Command, - path: Vec, - paths: &mut HashMap>, - ovd_interfaces: &mut BTreeMap, - ) -> clap::Command { - let id = path.join("."); - let prev = paths.insert(clap::Id::from(&id), path.clone()); - debug_assert!(prev.is_none()); - - // this is a terminal field, which gives rise to an argument or an overridable value. + fn add_args(&self, mut term_cmd: TermCommand, path: Vec) -> TermCommand { + // This is a terminal field, which gives rise to an argument or an overridable value. if !self.has_subfields() { if self.is_input() { - cmd = self.add_arg(cmd, id); + match path.as_slice() { + [ref field] if field == "override" || field == "help" => { + term_cmd.register_input(path.clone()) + } + _ => { + term_cmd.register_arg(path.clone()); + term_cmd.clap_cmd = self.add_arg(term_cmd.clap_cmd, path.join(".")) + } + }; } else { - ovd_interfaces.insert( - id, - OverrideInterface { - path: path.clone(), - _help: self.help(), - }, - ); + term_cmd.register_override(path.clone(), self.help()); } } for (id, field) in self.subfields.iter().flat_map(|intf| intf.fields.iter()) { let mut path = path.clone(); path.push(id.to_string()); - cmd = field.add_args(cmd, path, paths, ovd_interfaces); + + term_cmd = field.add_args(term_cmd, path); } - cmd + term_cmd } /// Add a single argument to the CLI `cmd`, based on the interface of this field, and return @@ -454,10 +478,56 @@ struct TermCommand { clap_cmd: clap::Command, /// A mapping between clap argument ids and the corresponding field path (for inputs). args: HashMap>, + /// Mapping between existing input fields (as a dot-separated path string) and their path + /// represented as a vector. This is used both as a `HashSet` to check the existence of an + /// input field, and to sidestep the issue of parsing a path again when reading an input + /// argument. + inputs: HashMap>, /// A mapping between a field path and the corresponding override data. overrides: BTreeMap, } +impl TermCommand { + fn new(command: clap::Command) -> Self { + TermCommand { + clap_cmd: command, + overrides: BTreeMap::new(), + inputs: HashMap::new(), + args: HashMap::new(), + } + } + + /// Register a new argument corresponding to an input. Add the paths and the argument's id to + /// the relevant maps. + fn register_arg(&mut self, path: Vec) { + let path_string = path.join("."); + let arg_id = clap::Id::from(path_string.clone()); + + debug_assert!(!self.args.contains_key(&arg_id)); + debug_assert!(!self.inputs.contains_key(&path_string)); + + self.args.insert(arg_id, path.clone()); + self.inputs.insert(path_string, path); + } + + /// Register an input, but don't register the corresponding CLI argument. Used for `override` + /// and `help` which conflict with the built-in `--override` and `--help` arguments. + fn register_input(&mut self, path: Vec) { + let path_string = path.join("."); + + debug_assert!(!self.inputs.contains_key(&path_string)); + + self.inputs.insert(path_string, path); + } + + fn register_override(&mut self, path: Vec, help: Option) { + let path_string = path.join("."); + + self.overrides + .insert(path_string, OverrideInterface { path, _help: help }); + } +} + impl ExportCommand { pub fn run(mut self, global: GlobalOptions) -> CliResult<()> { let mut program = self.evaluation.prepare(&global)?; @@ -488,8 +558,13 @@ impl ExportCommand { cmd.overrides .get(path) - .map(|intf| FieldOverride { - path: intf.path.clone(), + .map(|intf| intf.path.clone()) + // We allow --override to set inputs as well, in particular as an + // escape mechanism to set an input field named `override` or `help`, + // which would conflict with the corresponding builtin flags. + .or(cmd.inputs.get(path).cloned()) + .map(|path| FieldOverride { + path, value: value.clone(), priority: MergePriority::Top, }) diff --git a/cli/tests/snapshot/inputs/customize-mode/has_help.ncl b/cli/tests/snapshot/inputs/customize-mode/has_help.ncl new file mode 100644 index 0000000000..289a4dcb23 --- /dev/null +++ b/cli/tests/snapshot/inputs/customize-mode/has_help.ncl @@ -0,0 +1,8 @@ +# capture = 'stdout' +# command = ['export', '--', '--help'] +{ + help | String, + input | Number, + + output = if input == 0 then help else "", +} diff --git a/cli/tests/snapshot/inputs/customize-mode/has_override.ncl b/cli/tests/snapshot/inputs/customize-mode/has_override.ncl new file mode 100644 index 0000000000..832cbc2df8 --- /dev/null +++ b/cli/tests/snapshot/inputs/customize-mode/has_override.ncl @@ -0,0 +1,8 @@ +# capture = 'stdout' +# command = ['export', '--', '--help'] +{ + override | String, + input | Number, + + output = if input == 0 then override else "", +} diff --git a/cli/tests/snapshot/inputs/freeform/help.ncl b/cli/tests/snapshot/inputs/customize-mode/help.ncl similarity index 100% rename from cli/tests/snapshot/inputs/freeform/help.ncl rename to cli/tests/snapshot/inputs/customize-mode/help.ncl diff --git a/cli/tests/snapshot/inputs/freeform/not_an_input.ncl b/cli/tests/snapshot/inputs/customize-mode/not_an_input.ncl similarity index 100% rename from cli/tests/snapshot/inputs/freeform/not_an_input.ncl rename to cli/tests/snapshot/inputs/customize-mode/not_an_input.ncl diff --git a/cli/tests/snapshot/inputs/freeform/simple_adder.ncl b/cli/tests/snapshot/inputs/customize-mode/simple_adder.ncl similarity index 100% rename from cli/tests/snapshot/inputs/freeform/simple_adder.ncl rename to cli/tests/snapshot/inputs/customize-mode/simple_adder.ncl diff --git a/cli/tests/snapshot/inputs/freeform/unknown_override.ncl b/cli/tests/snapshot/inputs/customize-mode/unknown_override.ncl similarity index 100% rename from cli/tests/snapshot/inputs/freeform/unknown_override.ncl rename to cli/tests/snapshot/inputs/customize-mode/unknown_override.ncl diff --git a/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_help.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_help.ncl.snap new file mode 100644 index 0000000000..feaf50259b --- /dev/null +++ b/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_help.ncl.snap @@ -0,0 +1,28 @@ +--- +source: cli/tests/snapshot/main.rs +expression: out +--- +Customize a Nickel configuration through the command line before exporting + +Usage: customize-mode [OPTIONS] --input + +Options: + --input + + + --override + Override any field of the configuration with a valid Nickel expression provided as a + string. The new value will be merged with the configuration with a `force` priority. + + Overridable fields: + - output + + -h, --help + Print help (see a summary with '-h') + +[CONFLICT] This configuration has a field named `help` which conflicts with the built-in `--help` +argument. To set this field to e.g. "some_value", use `--override help "some_value"` instead of +`--help "some_value"` + +[WARNING] Customize mode is experimental. Its interface is subject to breaking changes. + diff --git a/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_override.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_override.ncl.snap new file mode 100644 index 0000000000..78ef119172 --- /dev/null +++ b/cli/tests/snapshot/snapshots/snapshot__export_stdout_has_override.ncl.snap @@ -0,0 +1,28 @@ +--- +source: cli/tests/snapshot/main.rs +expression: out +--- +Customize a Nickel configuration through the command line before exporting + +Usage: customize-mode [OPTIONS] --input + +Options: + --input + + + --override + Override any field of the configuration with a valid Nickel expression provided as a + string. The new value will be merged with the configuration with a `force` priority. + + Overridable fields: + - output + + -h, --help + Print help (see a summary with '-h') + +[CONFLICT] This configuration has a field named `override` which conflicts with the built-in +`--override` argument. To set this field to e.g. "some_value", use `--override override +"some_value"` instead of `--override "some_value"` + +[WARNING] Customize mode is experimental. Its interface is subject to breaking changes. + diff --git a/cli/tests/snapshot/snapshots/snapshot__export_stdout_help.ncl.snap b/cli/tests/snapshot/snapshots/snapshot__export_stdout_help.ncl.snap index b8d65b4bf1..34a3453b05 100644 --- a/cli/tests/snapshot/snapshots/snapshot__export_stdout_help.ncl.snap +++ b/cli/tests/snapshot/snapshots/snapshot__export_stdout_help.ncl.snap @@ -2,6 +2,8 @@ source: cli/tests/snapshot/main.rs expression: out --- +Customize a Nickel configuration through the command line before exporting + Usage: customize-mode [OPTIONS] --input.foo.bar --input.foo.baz Options: @@ -23,3 +25,5 @@ Options: -h, --help Print help +[WARNING] Customize mode is experimental. Its interface is subject to breaking changes. +