cli: enrich the error about required template value with a hint

Several `jj` commands accept `--template <TEMPLATE>` argument. When the argument
is empty, `jj` will show the list of defined template aliases.
This commit is contained in:
Aleksey Kuznetsov 2024-03-03 11:04:59 +05:00
parent 8c0f6a53c5
commit 38d14eafe2
8 changed files with 172 additions and 13 deletions

View File

@ -90,6 +90,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Commands producing diffs now accept a `--context` flag for the number of * Commands producing diffs now accept a `--context` flag for the number of
lines of context to show. lines of context to show.
* `jj` commands with the `-T`/`--template` option now provide a hint containing
defined template names when no argument is given, assisting the user in making
a selection.
### Fixed bugs ### Fixed bugs
* On Windows, symlinks in the repo are now supported when Developer Mode is enabled. * On Windows, symlinks in the repo are now supported when Developer Mode is enabled.

View File

@ -28,6 +28,7 @@ use std::time::SystemTime;
use std::{fs, str}; use std::{fs, str};
use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory}; use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory};
use clap::error::{ContextKind, ContextValue};
use clap::{Arg, ArgAction, ArgMatches, Command, FromArgMatches}; use clap::{Arg, ArgAction, ArgMatches, Command, FromArgMatches};
use indexmap::{IndexMap, IndexSet}; use indexmap::{IndexMap, IndexSet};
use itertools::Itertools; use itertools::Itertools;
@ -2525,7 +2526,8 @@ impl CliRunner {
&self.tracing_subscription, &self.tracing_subscription,
&string_args, &string_args,
&mut layered_configs, &mut layered_configs,
)?; )
.map_err(|err| map_clap_cli_error(err, ui, &layered_configs))?;
for process_global_args_fn in self.process_global_args_fns { for process_global_args_fn in self.process_global_args_fns {
process_global_args_fn(ui, &matches)?; process_global_args_fn(ui, &matches)?;
} }
@ -2599,3 +2601,40 @@ impl CliRunner {
exit_code exit_code
} }
} }
fn map_clap_cli_error(
cmd_err: CommandError,
ui: &Ui,
layered_configs: &LayeredConfigs,
) -> CommandError {
let CommandError::ClapCliError { err, hint: None } = &cmd_err else {
return cmd_err;
};
if let (Some(ContextValue::String(arg)), Some(ContextValue::String(value))) = (
err.get(ContextKind::InvalidArg),
err.get(ContextKind::InvalidValue),
) {
if arg.as_str() == "--template <TEMPLATE>" && value.is_empty() {
// Suppress the error, it's less important than the original error.
if let Ok(template_aliases) = load_template_aliases(ui, layered_configs) {
return CommandError::ClapCliError {
err: err.clone(),
hint: Some(format_template_aliases_hint(&template_aliases)),
};
}
}
}
cmd_err
}
fn format_template_aliases_hint(template_aliases: &TemplateAliasesMap) -> String {
let mut hint = String::from("The following template aliases are defined:\n");
hint.push_str(
&template_aliases
.symbol_names()
.sorted_unstable()
.map(|name| format!("- {name}"))
.join("\n"),
);
hint
}

View File

@ -51,7 +51,10 @@ pub enum CommandError {
/// Invalid command line /// Invalid command line
CliError(String), CliError(String),
/// Invalid command line detected by clap /// Invalid command line detected by clap
ClapCliError(Arc<clap::Error>), ClapCliError {
err: Arc<clap::Error>,
hint: Option<String>,
},
BrokenPipe, BrokenPipe,
InternalError(Arc<dyn error::Error + Send + Sync>), InternalError(Arc<dyn error::Error + Send + Sync>),
} }
@ -414,7 +417,10 @@ impl From<FsPathParseError> for CommandError {
impl From<clap::Error> for CommandError { impl From<clap::Error> for CommandError {
fn from(err: clap::Error) -> Self { fn from(err: clap::Error) -> Self {
CommandError::ClapCliError(Arc::new(err)) CommandError::ClapCliError {
err: Arc::new(err),
hint: None,
}
} }
} }
@ -468,14 +474,14 @@ fn try_handle_command_result(
writeln!(ui.error(), "Error: {message}")?; writeln!(ui.error(), "Error: {message}")?;
Ok(ExitCode::from(2)) Ok(ExitCode::from(2))
} }
Err(CommandError::ClapCliError(inner)) => { Err(CommandError::ClapCliError { err, hint }) => {
let clap_str = if ui.color() { let clap_str = if ui.color() {
inner.render().ansi().to_string() err.render().ansi().to_string()
} else { } else {
inner.render().to_string() err.render().to_string()
}; };
match inner.kind() { match err.kind() {
clap::error::ErrorKind::DisplayHelp clap::error::ErrorKind::DisplayHelp
| clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => { | clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => {
ui.request_pager() ui.request_pager()
@ -484,17 +490,19 @@ fn try_handle_command_result(
}; };
// Definitions for exit codes and streams come from // Definitions for exit codes and streams come from
// https://github.com/clap-rs/clap/blob/master/src/error/mod.rs // https://github.com/clap-rs/clap/blob/master/src/error/mod.rs
match inner.kind() { match err.kind() {
clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion => { clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion => {
write!(ui.stdout(), "{clap_str}")?; write!(ui.stdout(), "{clap_str}")?;
Ok(ExitCode::SUCCESS) return Ok(ExitCode::SUCCESS);
}
_ => {}
} }
_ => {
write!(ui.stderr(), "{clap_str}")?; write!(ui.stderr(), "{clap_str}")?;
if let Some(hint) = hint {
writeln!(ui.hint(), "Hint: {hint}")?;
}
Ok(ExitCode::from(2)) Ok(ExitCode::from(2))
} }
}
}
Err(CommandError::BrokenPipe) => { Err(CommandError::BrokenPipe) => {
// A broken pipe is not an error, but a signal to exit gracefully. // A broken pipe is not an error, but a signal to exit gracefully.
Ok(ExitCode::from(BROKEN_PIPE_EXIT_CODE)) Ok(ExitCode::from(BROKEN_PIPE_EXIT_CODE))

View File

@ -561,6 +561,10 @@ impl TemplateAliasesMap {
Self::default() Self::default()
} }
pub fn symbol_names(&self) -> impl Iterator<Item = &str> {
self.symbol_aliases.keys().map(|s| s.as_str())
}
/// Adds new substitution rule `decl = defn`. /// Adds new substitution rule `decl = defn`.
/// ///
/// Returns error if `decl` is invalid. The `defn` part isn't checked. A bad /// Returns error if `decl` is invalid. The `defn` part isn't checked. A bad

View File

@ -28,6 +28,32 @@ fn test_log_with_empty_revision() {
"###); "###);
} }
#[test]
fn test_log_with_no_template() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["log", "-T"]);
insta::assert_snapshot!(stderr, @r###"
error: a value is required for '--template <TEMPLATE>' but none was supplied
For more information, try '--help'.
Hint: The following template aliases are defined:
- builtin_change_id_with_hidden_and_divergent_info
- builtin_log_comfortable
- builtin_log_compact
- builtin_log_detailed
- builtin_log_oneline
- builtin_op_log_comfortable
- builtin_op_log_compact
- commit_summary_separator
- description_placeholder
- email_placeholder
- name_placeholder
"###);
}
#[test] #[test]
fn test_log_with_or_without_diff() { fn test_log_with_or_without_diff() {
let test_env = TestEnvironment::default(); let test_env = TestEnvironment::default();

View File

@ -235,3 +235,29 @@ fn test_obslog_squash() {
(empty) second (empty) second
"###); "###);
} }
#[test]
fn test_obslog_with_no_template() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["obslog", "-T"]);
insta::assert_snapshot!(stderr, @r###"
error: a value is required for '--template <TEMPLATE>' but none was supplied
For more information, try '--help'.
Hint: The following template aliases are defined:
- builtin_change_id_with_hidden_and_divergent_info
- builtin_log_comfortable
- builtin_log_compact
- builtin_log_detailed
- builtin_log_oneline
- builtin_op_log_comfortable
- builtin_op_log_compact
- commit_summary_separator
- description_placeholder
- email_placeholder
- name_placeholder
"###);
}

View File

@ -93,6 +93,32 @@ fn test_op_log() {
"###); "###);
} }
#[test]
fn test_op_log_with_no_template() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["op", "log", "-T"]);
insta::assert_snapshot!(stderr, @r###"
error: a value is required for '--template <TEMPLATE>' but none was supplied
For more information, try '--help'.
Hint: The following template aliases are defined:
- builtin_change_id_with_hidden_and_divergent_info
- builtin_log_comfortable
- builtin_log_compact
- builtin_log_detailed
- builtin_log_oneline
- builtin_op_log_comfortable
- builtin_op_log_compact
- commit_summary_separator
- description_placeholder
- email_placeholder
- name_placeholder
"###);
}
#[test] #[test]
fn test_op_log_limit() { fn test_op_log_limit() {
let test_env = TestEnvironment::default(); let test_env = TestEnvironment::default();

View File

@ -48,6 +48,32 @@ fn test_show_with_template() {
"###); "###);
} }
#[test]
fn test_show_with_no_template() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
let stderr = test_env.jj_cmd_cli_error(&repo_path, &["show", "-T"]);
insta::assert_snapshot!(stderr, @r###"
error: a value is required for '--template <TEMPLATE>' but none was supplied
For more information, try '--help'.
Hint: The following template aliases are defined:
- builtin_change_id_with_hidden_and_divergent_info
- builtin_log_comfortable
- builtin_log_compact
- builtin_log_detailed
- builtin_log_oneline
- builtin_op_log_comfortable
- builtin_op_log_compact
- commit_summary_separator
- description_placeholder
- email_placeholder
- name_placeholder
"###);
}
#[test] #[test]
fn test_show_relative_timestamps() { fn test_show_relative_timestamps() {
let test_env = TestEnvironment::default(); let test_env = TestEnvironment::default();