From 074e6e12bc1bce406c705adef277936f65af8f69 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 26 Mar 2024 20:28:50 +0900 Subject: [PATCH] revset, templater: include short parse error description in summary line This makes the summary line more informative. Even though it just duplicates the message printed later, I think it's easier to follow. This patch also adjusts some RevsetParseError messages because it seemed redundant to repeat "revset function", "argument", etc. --- cli/src/command_error.rs | 6 ++- cli/tests/test_commit_template.rs | 10 ++-- cli/tests/test_revset_output.rs | 82 +++++++++++++++---------------- cli/tests/test_templater.rs | 28 +++++------ lib/src/revset.rs | 4 +- 5 files changed, 66 insertions(+), 64 deletions(-) diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index eb4c2a836..3c9eb4d9b 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -421,7 +421,8 @@ impl From for CommandError { } => format_similarity_hint(candidates), _ => None, }; - let mut cmd_err = user_error_with_message("Failed to parse revset", err); + let mut cmd_err = + user_error_with_message(format!("Failed to parse revset: {}", err.kind()), err); cmd_err.extend_hints(hint); cmd_err } @@ -467,7 +468,8 @@ impl From for CommandError { } _ => None, }; - let mut cmd_err = user_error_with_message("Failed to parse template", err); + let mut cmd_err = + user_error_with_message(format!("Failed to parse template: {}", err.kind()), err); cmd_err.extend_hints(hint); cmd_err } diff --git a/cli/tests/test_commit_template.rs b/cli/tests/test_commit_template.rs index 9c64e17e1..26111a3a2 100644 --- a/cli/tests/test_commit_template.rs +++ b/cli/tests/test_commit_template.rs @@ -54,7 +54,7 @@ fn test_log_parents() { // Commit object isn't printable let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-T", "parents"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse template + Error: Failed to parse template: Expected expression of type "Template" Caused by: --> 1:1 | 1 | parents @@ -67,7 +67,7 @@ fn test_log_parents() { let template = r#"parents.map(|c| c.commit_id(""))"#; let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-T", template]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse template + Error: Failed to parse template: Function "commit_id": Expected 0 arguments Caused by: --> 1:29 | 1 | parents.map(|c| c.commit_id("")) @@ -599,7 +599,7 @@ fn test_log_immutable() { test_env.add_config("revset-aliases.'immutable_heads()' = 'unknown_fn()'"); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r::", "-T", template]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse template + Error: Failed to parse template: Failed to parse revset Caused by: 1: --> 5:10 | @@ -612,13 +612,13 @@ fn test_log_immutable() { 1 | unknown_fn() | ^--------^ | - = Revset function "unknown_fn" doesn't exist + = Function "unknown_fn" doesn't exist "###); test_env.add_config("revset-aliases.'immutable_heads()' = 'unknown_symbol'"); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r::", "-T", template]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse template + Error: Failed to parse template: Failed to evaluate revset Caused by: 1: --> 5:10 | diff --git a/cli/tests/test_revset_output.rs b/cli/tests/test_revset_output.rs index 4d17b4ec6..3d8260b95 100644 --- a/cli/tests/test_revset_output.rs +++ b/cli/tests/test_revset_output.rs @@ -22,7 +22,7 @@ fn test_syntax_error() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", ":x"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: ':' is not a prefix operator Caused by: --> 1:1 | 1 | :x @@ -34,7 +34,7 @@ fn test_syntax_error() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "x &"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Syntax error Caused by: --> 1:4 | 1 | x & @@ -45,7 +45,7 @@ fn test_syntax_error() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "x - y"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: '-' is not an infix operator Caused by: --> 1:3 | 1 | x - y @@ -57,7 +57,7 @@ fn test_syntax_error() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "HEAD^"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: '^' is not a postfix operator Caused by: --> 1:5 | 1 | HEAD^ @@ -76,84 +76,84 @@ fn test_bad_function_call() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "all(or::nothing)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "all": Expected 0 arguments Caused by: --> 1:5 | 1 | all(or::nothing) | ^---------^ | - = Invalid arguments to revset function "all": Expected 0 arguments + = Function "all": Expected 0 arguments "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "parents()"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "parents": Expected 1 arguments Caused by: --> 1:9 | 1 | parents() | ^ | - = Invalid arguments to revset function "parents": Expected 1 arguments + = Function "parents": Expected 1 arguments "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "parents(foo, bar)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "parents": Expected 1 arguments Caused by: --> 1:9 | 1 | parents(foo, bar) | ^------^ | - = Invalid arguments to revset function "parents": Expected 1 arguments + = Function "parents": Expected 1 arguments "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "heads(foo, bar)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "heads": Expected 1 arguments Caused by: --> 1:7 | 1 | heads(foo, bar) | ^------^ | - = Invalid arguments to revset function "heads": Expected 1 arguments + = Function "heads": Expected 1 arguments "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "latest(a, not_an_integer)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "latest": Expected function argument of type integer Caused by: --> 1:11 | 1 | latest(a, not_an_integer) | ^------------^ | - = Invalid arguments to revset function "latest": Expected function argument of type integer + = Function "latest": Expected function argument of type integer "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file()"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "file": Expected at least 1 argument Caused by: --> 1:6 | 1 | file() | ^ | - = Invalid arguments to revset function "file": Expected at least 1 argument + = Function "file": Expected at least 1 argument "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "file(a, not:a-string)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "file": Expected function argument of type string Caused by: --> 1:9 | 1 | file(a, not:a-string) | ^----------^ | - = Invalid arguments to revset function "file": Expected function argument of type string + = Function "file": Expected function argument of type string "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", r#"file(a, "../out")"#]); insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Invalid file pattern Caused by: 1: --> 1:9 | @@ -167,24 +167,24 @@ fn test_bad_function_call() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "branches(bad:pattern)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "branches": Invalid string pattern kind "bad:", try prefixing with one of `exact:`, `glob:` or `substring:` Caused by: --> 1:10 | 1 | branches(bad:pattern) | ^---------^ | - = Invalid arguments to revset function "branches": Invalid string pattern kind "bad:", try prefixing with one of `exact:`, `glob:` or `substring:` + = Function "branches": Invalid string pattern kind "bad:", try prefixing with one of `exact:`, `glob:` or `substring:` "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "root()::whatever()"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "whatever" doesn't exist Caused by: --> 1:9 | 1 | root()::whatever() | ^------^ | - = Revset function "whatever" doesn't exist + = Function "whatever" doesn't exist "###); let stderr = test_env.jj_cmd_failure( @@ -192,30 +192,30 @@ fn test_bad_function_call() { &["log", "-r", "remote_branches(a, b, remote=c)"], ); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "remote_branches": Got multiple values for keyword "remote" Caused by: --> 1:23 | 1 | remote_branches(a, b, remote=c) | ^------^ | - = Invalid arguments to revset function "remote_branches": Got multiple values for keyword "remote" + = Function "remote_branches": Got multiple values for keyword "remote" "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(remote=a, b)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "remote_branches": Positional argument follows keyword argument Caused by: --> 1:27 | 1 | remote_branches(remote=a, b) | ^ | - = Invalid arguments to revset function "remote_branches": Positional argument follows keyword argument + = Function "remote_branches": Positional argument follows keyword argument "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(=foo)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Syntax error Caused by: --> 1:17 | 1 | remote_branches(=foo) @@ -226,7 +226,7 @@ fn test_bad_function_call() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "remote_branches(remote=)"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Syntax error Caused by: --> 1:24 | 1 | remote_branches(remote=) @@ -255,30 +255,30 @@ fn test_function_name_hint() { // The suggestion "branches" shouldn't be duplicated insta::assert_snapshot!(evaluate_err("branch()"), @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "branch" doesn't exist Caused by: --> 1:1 | 1 | branch() | ^----^ | - = Revset function "branch" doesn't exist + = Function "branch" doesn't exist Hint: Did you mean "branches"? "###); // Both builtin function and function alias should be suggested insta::assert_snapshot!(evaluate_err("author_()"), @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "author_" doesn't exist Caused by: --> 1:1 | 1 | author_() | ^-----^ | - = Revset function "author_" doesn't exist + = Function "author_" doesn't exist Hint: Did you mean "author", "my_author"? "###); insta::assert_snapshot!(evaluate_err("my_branches"), @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Alias "my_branches" cannot be expanded Caused by: 1: --> 1:1 | @@ -291,7 +291,7 @@ fn test_function_name_hint() { 1 | branch() | ^----^ | - = Revset function "branch" doesn't exist + = Function "branch" doesn't exist Hint: Did you mean "branches"? "###); } @@ -327,7 +327,7 @@ fn test_alias() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "root() & syntax-error"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Alias "syntax-error" cannot be expanded Caused by: 1: --> 1:10 | @@ -345,18 +345,18 @@ fn test_alias() { let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "identity()"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Function "identity": Expected 1 arguments Caused by: --> 1:10 | 1 | identity() | ^ | - = Invalid arguments to revset function "identity": Expected 1 arguments + = Function "identity": Expected 1 arguments "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "my_author(none())"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Alias "my_author()" cannot be expanded Caused by: 1: --> 1:1 | @@ -369,12 +369,12 @@ fn test_alias() { 1 | author(x) | ^ | - = Invalid arguments to revset function "author": Expected function argument of string pattern + = Function "author": Expected function argument of string pattern "###); let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r", "root() & recurse"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to parse revset + Error: Failed to parse revset: Alias "recurse" cannot be expanded Caused by: 1: --> 1:10 | diff --git a/cli/tests/test_templater.rs b/cli/tests/test_templater.rs index 9248177fb..cc076c9fe 100644 --- a/cli/tests/test_templater.rs +++ b/cli/tests/test_templater.rs @@ -24,7 +24,7 @@ fn test_templater_parse_error() { let render_err = |template| test_env.jj_cmd_failure(&repo_path, &["log", "-T", template]); insta::assert_snapshot!(render_err(r#"description ()"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Syntax error Caused by: --> 1:13 | 1 | description () @@ -44,7 +44,7 @@ fn test_templater_parse_error() { "###, ); insta::assert_snapshot!(render_err(r#"conflicts"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Keyword "conflicts" doesn't exist Caused by: --> 1:1 | 1 | conflicts @@ -54,7 +54,7 @@ fn test_templater_parse_error() { Hint: Did you mean "conflict", "conflicting"? "###); insta::assert_snapshot!(render_err(r#"commit_id.shorter()"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Method "shorter" doesn't exist for type "CommitOrChangeId" Caused by: --> 1:11 | 1 | commit_id.shorter() @@ -64,7 +64,7 @@ fn test_templater_parse_error() { Hint: Did you mean "short", "shortest"? "###); insta::assert_snapshot!(render_err(r#"oncat()"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Function "oncat" doesn't exist Caused by: --> 1:1 | 1 | oncat() @@ -74,7 +74,7 @@ fn test_templater_parse_error() { Hint: Did you mean "concat", "socat"? "###); insta::assert_snapshot!(render_err(r#""".lines().map(|s| se)"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Keyword "se" doesn't exist Caused by: --> 1:20 | 1 | "".lines().map(|s| se) @@ -84,7 +84,7 @@ fn test_templater_parse_error() { Hint: Did you mean "s", "self"? "###); insta::assert_snapshot!(render_err(r#"format_id(commit_id)"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "format_id()" cannot be expanded Caused by: 1: --> 1:1 | @@ -104,7 +104,7 @@ fn test_templater_parse_error() { // -Tbuiltin shows the predefined builtin_* aliases. This isn't 100% // guaranteed, but is nice. insta::assert_snapshot!(render_err(r#"builtin"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Keyword "builtin" doesn't exist Caused by: --> 1:1 | 1 | builtin @@ -155,7 +155,7 @@ fn test_templater_alias() { insta::assert_snapshot!(render("identity(my_commit_id)"), @"000000000000"); insta::assert_snapshot!(render_err("commit_id ++ syntax_error"), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "syntax_error" cannot be expanded Caused by: 1: --> 1:14 | @@ -172,7 +172,7 @@ fn test_templater_alias() { "###); insta::assert_snapshot!(render_err("commit_id ++ name_error"), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "name_error" cannot be expanded Caused by: 1: --> 1:14 | @@ -189,7 +189,7 @@ fn test_templater_alias() { "###); insta::assert_snapshot!(render_err(r#"identity(identity(commit_id.short("")))"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "identity()" cannot be expanded Caused by: 1: --> 1:1 | @@ -212,7 +212,7 @@ fn test_templater_alias() { "###); insta::assert_snapshot!(render_err("commit_id ++ recurse"), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "recurse" cannot be expanded Caused by: 1: --> 1:14 | @@ -241,7 +241,7 @@ fn test_templater_alias() { "###); insta::assert_snapshot!(render_err("identity()"), @r###" - Error: Failed to parse template + Error: Failed to parse template: Function "identity": Expected 1 arguments Caused by: --> 1:10 | 1 | identity() @@ -250,7 +250,7 @@ fn test_templater_alias() { = Function "identity": Expected 1 arguments "###); insta::assert_snapshot!(render_err("identity(commit_id, commit_id)"), @r###" - Error: Failed to parse template + Error: Failed to parse template: Function "identity": Expected 1 arguments Caused by: --> 1:10 | 1 | identity(commit_id, commit_id) @@ -260,7 +260,7 @@ fn test_templater_alias() { "###); insta::assert_snapshot!(render_err(r#"coalesce(label("x", "not boolean"), "")"#), @r###" - Error: Failed to parse template + Error: Failed to parse template: Alias "coalesce()" cannot be expanded Caused by: 1: --> 1:1 | diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 0c7b197fd..040553663 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -171,12 +171,12 @@ pub enum RevsetParseErrorKind { similar_op: String, description: String, }, - #[error(r#"Revset function "{name}" doesn't exist"#)] + #[error(r#"Function "{name}" doesn't exist"#)] NoSuchFunction { name: String, candidates: Vec, }, - #[error("Invalid arguments to revset function \"{name}\": {message}")] + #[error(r#"Function "{name}": {message}"#)] InvalidFunctionArguments { name: String, message: String }, #[error("Invalid file pattern")] FsPathParseError,