Skip to content

Commit

Permalink
cli: enrich the error about required template value with a hint
Browse files Browse the repository at this point in the history
Several `jj` commands accept `--template <TEMPLATE>` argument. When the argument
is empty, `jj` will show the list of defined template aliases.
  • Loading branch information
zummenix committed Mar 5, 2024
1 parent fb5fe5c commit 551113f
Show file tree
Hide file tree
Showing 8 changed files with 172 additions and 13 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,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
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

* On Windows, symlinks in the repo are now supported when Developer Mode is enabled.
Expand Down
41 changes: 40 additions & 1 deletion cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use std::time::SystemTime;
use std::{fs, str};

use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory};
use clap::error::{ContextKind, ContextValue};
use clap::{Arg, ArgAction, ArgMatches, Command, FromArgMatches};
use indexmap::{IndexMap, IndexSet};
use itertools::Itertools;
Expand Down Expand Up @@ -2525,7 +2526,8 @@ impl CliRunner {
&self.tracing_subscription,
&string_args,
&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 {
process_global_args_fn(ui, &matches)?;
}
Expand Down Expand Up @@ -2599,3 +2601,40 @@ impl CliRunner {
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
}
32 changes: 20 additions & 12 deletions cli/src/command_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ pub enum CommandError {
/// Invalid command line
CliError(String),
/// Invalid command line detected by clap
ClapCliError(Arc<clap::Error>),
ClapCliError {
err: Arc<clap::Error>,
hint: Option<String>,
},
BrokenPipe,
InternalError(Arc<dyn error::Error + Send + Sync>),
}
Expand Down Expand Up @@ -414,7 +417,10 @@ impl From<FsPathParseError> for CommandError {

impl From<clap::Error> for CommandError {
fn from(err: clap::Error) -> Self {
CommandError::ClapCliError(Arc::new(err))
CommandError::ClapCliError {
err: Arc::new(err),
hint: None,
}
}
}

Expand Down Expand Up @@ -468,14 +474,14 @@ fn try_handle_command_result(
writeln!(ui.error(), "Error: {message}")?;
Ok(ExitCode::from(2))
}
Err(CommandError::ClapCliError(inner)) => {
Err(CommandError::ClapCliError { err, hint }) => {
let clap_str = if ui.color() {
inner.render().ansi().to_string()
err.render().ansi().to_string()
} else {
inner.render().to_string()
err.render().to_string()
};

match inner.kind() {
match err.kind() {
clap::error::ErrorKind::DisplayHelp
| clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => {
ui.request_pager()
Expand All @@ -484,16 +490,18 @@ fn try_handle_command_result(
};
// Definitions for exit codes and streams come from
// 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 => {
write!(ui.stdout(), "{clap_str}")?;
Ok(ExitCode::SUCCESS)
}
_ => {
write!(ui.stderr(), "{clap_str}")?;
Ok(ExitCode::from(2))
return Ok(ExitCode::SUCCESS);
}
_ => {}
}
write!(ui.stderr(), "{clap_str}")?;
if let Some(hint) = hint {
writeln!(ui.hint(), "Hint: {hint}")?;
}
Ok(ExitCode::from(2))
}
Err(CommandError::BrokenPipe) => {
// A broken pipe is not an error, but a signal to exit gracefully.
Expand Down
4 changes: 4 additions & 0 deletions cli/src/template_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,10 @@ impl TemplateAliasesMap {
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`.
///
/// Returns error if `decl` is invalid. The `defn` part isn't checked. A bad
Expand Down
26 changes: 26 additions & 0 deletions cli/tests/test_log_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
fn test_log_with_or_without_diff() {
let test_env = TestEnvironment::default();
Expand Down
26 changes: 26 additions & 0 deletions cli/tests/test_obslog_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,29 @@ fn test_obslog_squash() {
(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
"###);
}
26 changes: 26 additions & 0 deletions cli/tests/test_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
fn test_op_log_limit() {
let test_env = TestEnvironment::default();
Expand Down
26 changes: 26 additions & 0 deletions cli/tests/test_show_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
fn test_show_relative_timestamps() {
let test_env = TestEnvironment::default();
Expand Down

0 comments on commit 551113f

Please sign in to comment.