Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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