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.

Sign up for GitHub

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: improve discoverability of template-aliases #3189

Closed
wants to merge 1 commit into from
Closed

cli: improve discoverability of template-aliases #3189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions cli/src/command_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ use jj_lib::working_copy::{ResetError, SnapshotError, WorkingCopyStateError};
use jj_lib::workspace::WorkspaceInitError;
use thiserror::Error;

use crate::cli_util::WorkspaceCommandHelper;
use crate::merge_tools::{
ConflictResolveError, DiffEditError, DiffGenerateError, MergeToolConfigError,
};
Expand Down Expand Up @@ -76,6 +77,24 @@ impl ErrorWithMessage {
}
}

pub fn user_error_missing_template(workspace_command: &WorkspaceCommandHelper) -> CommandError {
let mut template_names = workspace_command
.template_aliases_map()
.symbol_aliases_keys();
template_names.sort_unstable();
let mut hint = String::new();
let padding = " ";
for name in template_names {
hint.push('\n');
hint.push_str(padding);
hint.push_str(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: something like writeln!(hint, " {name}").unwrap() also works. Or simply .map(|name| format!(..)).join("\n").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format! will allocate a new string. It's not that important here of course, but still... I think writeln!(hint, " {name}").unwrap() will be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It's not important here, but clippy might complain about that.

}
user_error_with_hint(
String::from("Template is required"),
format!("The following template-aliases are defined:{}", hint),
)
}

pub fn user_error(err: impl Into<Box<dyn error::Error + Send + Sync>>) -> CommandError {
user_error_with_hint_opt(err, None)
}
Expand Down
7 changes: 4 additions & 3 deletions cli/src/commands/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use jj_lib::revset_graph::{
use tracing::instrument;

use crate::cli_util::{CommandHelper, LogContentFormat, RevisionArg};
use crate::command_error::CommandError;
use crate::command_error::{user_error_missing_template, CommandError};
use crate::diff_util::{self, DiffFormatArgs};
use crate::graphlog::{get_graphlog, Edge};
use crate::ui::Ui;
Expand Down Expand Up @@ -52,7 +52,7 @@ pub(crate) struct LogArgs {
///
/// For the syntax, see https://github.com/martinvonz/jj/blob/main/docs/templates.md
#[arg(long, short = 'T')]
template: Option<String>,
template: Option<Option<String>>,
/// Show patch
#[arg(long, short = 'p')]
patch: bool,
Expand Down Expand Up @@ -101,7 +101,8 @@ pub(crate) fn cmd_log(
diff_util::diff_formats_for_log(command.settings(), &args.diff_format, args.patch)?;

let template_string = match &args.template {
Some(value) => value.to_string(),
Some(Some(value)) => value.to_string(),
Some(None) => return Err(user_error_missing_template(&workspace_command)),
None => command.settings().config().get_string("templates.log")?,
};
let use_elided_nodes = command
Expand Down
4 changes: 4 additions & 0 deletions cli/src/template_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,10 @@ impl TemplateAliasesMap {
Self::default()
}

pub fn symbol_aliases_keys(&self) -> Vec<&str> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: symbol_names or symbol_alias_names ("key" sounds more like an implementation detail.)

self.symbol_aliases.keys().map(|s| s.as_str()).collect_vec()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: simply return impl Iterator<Item = &str> as the caller need to post-process the items anyway?

}

/// Adds new substitution rule `decl = defn`.
///
/// Returns error if `decl` is invalid. The `defn` part isn't checked. A bad
Expand Down
2 changes: 1 addition & 1 deletion cli/tests/test_log_command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ fn test_log_with_or_without_diff() {
insta::assert_snapshot!(stderr, @r###"
error: the argument '--git' cannot be used with '--color-words'

Usage: jj log --template <TEMPLATE> --no-graph --patch --git [PATHS]...
Usage: jj log --template [<TEMPLATE>] --no-graph --patch --git [PATHS]...
Copy link
Contributor

@yuja yuja Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that the --template value has to be optional.

Another implementation idea is to inspect the clap::Error at handle_command_result() or CliRunner::run_internal().

[cli/src/command_error.rs:478:13] (inner.kind(), inner.context().collect_vec()) = (
    InvalidValue,
    [
        (
            InvalidArg,
            String(
                "--template <TEMPLATE>",
            ),
        ),
        (
            InvalidValue,
            String(
                "",
            ),
        ),
        (
            ValidValue,
            Strings(
                [],
            ),
        ),
    ],
)

Doing that seems intrusive, but I think it's okay because --template is the concept apply to many commands. A downside is that we'll need to build template aliases from the config object available at that point (because WorkspaceCommandHelper isn't instantiated yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented this variant in #3200. Please take a look.


For more information, try '--help'.
"###);
Expand Down