cli: improve discoverability of template-aliases #3189

zummenix · 2024-03-02T15:24:03Z

This is follow up of #3175. This is a draft. Looking for initial feedback.

When user types jj log -T without passing a template name we can list all of them:

Error: Template is required
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

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

joyously · 2024-03-02T17:06:07Z

This looks useful at first glance.
When I looked closer, I wondered if the file name could be output, so the definitions could be examined.
And then I wondered if templates work for any command or just the one it's designed for (email_placeholder on a status command).

zummenix · 2024-03-02T17:31:15Z

When I looked closer, I wondered if the file name could be output, so the definitions could be examined.

I'm not sure about the file name but one can explore a template using jj config get command, for example jj config get template-aliases.builtin_op_log_compact. Perhaps, we should mention that in the hint?

And then I wondered if templates work for any command or just the one it's designed for (email_placeholder on a status command).

Currently, there is no connection between templates and their corresponding commands. For instance, using builtin_op_log_compact for jj log fails with an error:

$ jj log -T builtin_op_log_compact
Error: Failed to parse template:  --> 1:1
  |
1 | builtin_op_log_compact
  | ^--------------------^
  |
  = Alias "builtin_op_log_compact" cannot be expanded
 --> 2:3
  |
2 |   builtin_op_log_root(id),
  |   ^---------------------^
  |
  = Alias "builtin_op_log_root()" cannot be expanded
 --> 2:23
  |
2 |   builtin_op_log_root(id),
  |                       ^^
  |
  = Keyword "id" doesn't exist
Hint: Did you mean "hidden"?

When user types `jj log -T` without passing a template name we can list all of them

yuja

Seems useful.

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

Not all aliases are supposed to be used as a top-level template, but I have no idea how we can improve that. We could use a static list of builtin templates, but that means user aliases wouldn't be included.

yuja · 2024-03-03T04:52:59Z

cli/src/command_error.rs

+    for name in template_names {
+        hint.push('\n');
+        hint.push_str(padding);
+        hint.push_str(name);


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

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.

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

yuja · 2024-03-03T04:55:23Z

cli/src/template_parser.rs

@@ -561,6 +561,10 @@ impl TemplateAliasesMap {
        Self::default()
    }

+    pub fn symbol_aliases_keys(&self) -> Vec<&str> {
+        self.symbol_aliases.keys().map(|s| s.as_str()).collect_vec()


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

yuja · 2024-03-03T04:56:13Z

cli/src/template_parser.rs

@@ -561,6 +561,10 @@ impl TemplateAliasesMap {
        Self::default()
    }

+    pub fn symbol_aliases_keys(&self) -> Vec<&str> {


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

yuja · 2024-03-03T05:04:37Z

cli/tests/test_log_command.rs

@@ -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]...


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.

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

zummenix · 2024-03-03T05:49:09Z

Not all aliases are supposed to be used as a top-level template, but I have no idea how we can improve that. We could use a static list of builtin templates, but that means user aliases wouldn't be included.

Yeah, I think it's important to include user templates in the output.

On improvement: I was thinking maybe we could have some kind of introspection for templates to extract "features" and match these set of "features" with the set of "features" of a command. In my opinion, this greatly overcomplicate things and maybe not worth it in this context.

zummenix · 2024-03-04T04:31:44Z

Closing in favor of #3200

cli: improve discoverability of template-aliases

ff4d0c3

When user types `jj log -T` without passing a template name we can list all of them

zummenix force-pushed the template-aliases-hint branch from 5b33309 to ff4d0c3 Compare March 2, 2024 17:52

yuja reviewed Mar 3, 2024

View reviewed changes

zummenix mentioned this pull request Mar 3, 2024

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

Merged

4 tasks

zummenix closed this Mar 4, 2024

zummenix deleted the template-aliases-hint branch March 4, 2024 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: improve discoverability of template-aliases #3189

cli: improve discoverability of template-aliases #3189

zummenix commented Mar 2, 2024

joyously commented Mar 2, 2024

zummenix commented Mar 2, 2024 •

edited

Loading

yuja left a comment

yuja Mar 3, 2024

zummenix Mar 3, 2024

yuja Mar 3, 2024

yuja Mar 3, 2024

yuja Mar 3, 2024

yuja Mar 3, 2024 •

edited

Loading

zummenix Mar 3, 2024

zummenix commented Mar 3, 2024

zummenix commented Mar 4, 2024

cli: improve discoverability of template-aliases #3189

cli: improve discoverability of template-aliases #3189

Conversation

zummenix commented Mar 2, 2024

Checklist

joyously commented Mar 2, 2024

zummenix commented Mar 2, 2024 • edited Loading

yuja left a comment

Choose a reason for hiding this comment

yuja Mar 3, 2024

Choose a reason for hiding this comment

zummenix Mar 3, 2024

Choose a reason for hiding this comment

yuja Mar 3, 2024

Choose a reason for hiding this comment

yuja Mar 3, 2024

Choose a reason for hiding this comment

yuja Mar 3, 2024

Choose a reason for hiding this comment

yuja Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

zummenix Mar 3, 2024

Choose a reason for hiding this comment

zummenix commented Mar 3, 2024

zummenix commented Mar 4, 2024

zummenix commented Mar 2, 2024 •

edited

Loading

yuja Mar 3, 2024 •

edited

Loading