From 7399c272c237f1d9f3f5ab6f5280c8bdac459d56 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 19 Apr 2023 09:14:50 +0800 Subject: [PATCH 1/3] Better error message when getting an empty dep table Signed-off-by: hi-rustin --- src/cargo/util/toml_mut/dependency.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/cargo/util/toml_mut/dependency.rs b/src/cargo/util/toml_mut/dependency.rs index d8a2f2750aa..cb375cc8d17 100644 --- a/src/cargo/util/toml_mut/dependency.rs +++ b/src/cargo/util/toml_mut/dependency.rs @@ -279,7 +279,13 @@ impl Dependency { let src = WorkspaceSource::new(); src.into() } else { - anyhow::bail!("Unrecognized dependency source for `{key}`"); + let mut msg = format!("Unrecognized dependency source for `{key}`"); + if table.is_empty() { + msg.push_str( + ", expected a table with a `version`, `git`, `path`, or `workspace` key", + ); + } + anyhow::bail!(msg); }; let registry = if let Some(value) = table.get("registry") { Some( From 4d401bd0b9f0ea5dc40284422440682ef45e2717 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 19 Apr 2023 09:39:33 +0800 Subject: [PATCH 2/3] Add test for empty dep table error Signed-off-by: hi-rustin --- .../cargo_add/empty_dep_table/in/Cargo.toml | 8 ++++++ .../cargo_add/empty_dep_table/in/src/lib.rs | 1 + .../cargo_add/empty_dep_table/mod.rs | 25 +++++++++++++++++++ .../cargo_add/empty_dep_table/out/Cargo.toml | 8 ++++++ .../cargo_add/empty_dep_table/stderr.log | 1 + .../cargo_add/empty_dep_table/stdout.log | 0 tests/testsuite/cargo_add/mod.rs | 1 + 7 files changed, 44 insertions(+) create mode 100644 tests/testsuite/cargo_add/empty_dep_table/in/Cargo.toml create mode 100644 tests/testsuite/cargo_add/empty_dep_table/in/src/lib.rs create mode 100644 tests/testsuite/cargo_add/empty_dep_table/mod.rs create mode 100644 tests/testsuite/cargo_add/empty_dep_table/out/Cargo.toml create mode 100644 tests/testsuite/cargo_add/empty_dep_table/stderr.log create mode 100644 tests/testsuite/cargo_add/empty_dep_table/stdout.log diff --git a/tests/testsuite/cargo_add/empty_dep_table/in/Cargo.toml b/tests/testsuite/cargo_add/empty_dep_table/in/Cargo.toml new file mode 100644 index 00000000000..cac16bb9472 --- /dev/null +++ b/tests/testsuite/cargo_add/empty_dep_table/in/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dependencies] +your-face = { } diff --git a/tests/testsuite/cargo_add/empty_dep_table/in/src/lib.rs b/tests/testsuite/cargo_add/empty_dep_table/in/src/lib.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/tests/testsuite/cargo_add/empty_dep_table/in/src/lib.rs @@ -0,0 +1 @@ + diff --git a/tests/testsuite/cargo_add/empty_dep_table/mod.rs b/tests/testsuite/cargo_add/empty_dep_table/mod.rs new file mode 100644 index 00000000000..f6c507188ad --- /dev/null +++ b/tests/testsuite/cargo_add/empty_dep_table/mod.rs @@ -0,0 +1,25 @@ +use cargo_test_support::compare::assert_ui; +use cargo_test_support::prelude::*; +use cargo_test_support::Project; + +use crate::cargo_add::init_registry; +use cargo_test_support::curr_dir; + +#[cargo_test] +fn case() { + init_registry(); + let project = Project::from_template(curr_dir!().join("in")); + let project_root = project.root(); + let cwd = &project_root; + + snapbox::cmd::Command::cargo_ui() + .arg("add") + .arg_line("your-face --features eyes") + .current_dir(cwd) + .assert() + .code(101) + .stdout_matches_path(curr_dir!().join("stdout.log")) + .stderr_matches_path(curr_dir!().join("stderr.log")); + + assert_ui().subset_matches(curr_dir!().join("out"), &project_root); +} diff --git a/tests/testsuite/cargo_add/empty_dep_table/out/Cargo.toml b/tests/testsuite/cargo_add/empty_dep_table/out/Cargo.toml new file mode 100644 index 00000000000..cac16bb9472 --- /dev/null +++ b/tests/testsuite/cargo_add/empty_dep_table/out/Cargo.toml @@ -0,0 +1,8 @@ +[workspace] + +[package] +name = "cargo-list-test-fixture" +version = "0.0.0" + +[dependencies] +your-face = { } diff --git a/tests/testsuite/cargo_add/empty_dep_table/stderr.log b/tests/testsuite/cargo_add/empty_dep_table/stderr.log new file mode 100644 index 00000000000..00b1db5e369 --- /dev/null +++ b/tests/testsuite/cargo_add/empty_dep_table/stderr.log @@ -0,0 +1 @@ +error: Unrecognized dependency source for `your-face`, expected a table with a `version`, `git`, `path`, or `workspace` key diff --git a/tests/testsuite/cargo_add/empty_dep_table/stdout.log b/tests/testsuite/cargo_add/empty_dep_table/stdout.log new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/testsuite/cargo_add/mod.rs b/tests/testsuite/cargo_add/mod.rs index ca58474d219..26a11dd8a49 100644 --- a/tests/testsuite/cargo_add/mod.rs +++ b/tests/testsuite/cargo_add/mod.rs @@ -15,6 +15,7 @@ mod dev; mod dev_build_conflict; mod dev_prefer_existing_version; mod dry_run; +mod empty_dep_table; mod features; mod features_empty; mod features_multiple_occurrences; From e0276cae19729320f0af02800f415585ced4061e Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Thu, 20 Apr 2023 08:52:10 +0800 Subject: [PATCH 3/3] Update error message to not start with capital letters Signed-off-by: hi-rustin --- src/cargo/util/toml_mut/dependency.rs | 104 +++++++++--------- .../cargo_add/empty_dep_table/stderr.log | 2 +- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/cargo/util/toml_mut/dependency.rs b/src/cargo/util/toml_mut/dependency.rs index cb375cc8d17..1b24833c1ee 100644 --- a/src/cargo/util/toml_mut/dependency.rs +++ b/src/cargo/util/toml_mut/dependency.rs @@ -225,68 +225,72 @@ impl Dependency { (key.to_owned(), None) }; - let source: Source = - if let Some(git) = table.get("git") { - let mut src = GitSource::new( - git.as_str() - .ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?, - ); - if let Some(value) = table.get("branch") { - src = src.set_branch(value.as_str().ok_or_else(|| { + let source: Source = if let Some(git) = table.get("git") { + let mut src = GitSource::new( + git.as_str() + .ok_or_else(|| invalid_type(key, "git", git.type_name(), "string"))?, + ); + if let Some(value) = table.get("branch") { + src = + src.set_branch(value.as_str().ok_or_else(|| { invalid_type(key, "branch", value.type_name(), "string") })?); - } - if let Some(value) = table.get("tag") { - src = src.set_tag(value.as_str().ok_or_else(|| { + } + if let Some(value) = table.get("tag") { + src = + src.set_tag(value.as_str().ok_or_else(|| { invalid_type(key, "tag", value.type_name(), "string") })?); - } - if let Some(value) = table.get("rev") { - src = src.set_rev(value.as_str().ok_or_else(|| { + } + if let Some(value) = table.get("rev") { + src = + src.set_rev(value.as_str().ok_or_else(|| { invalid_type(key, "rev", value.type_name(), "string") })?); - } - if let Some(value) = table.get("version") { - src = src.set_version(value.as_str().ok_or_else(|| { - invalid_type(key, "version", value.type_name(), "string") - })?); - } - src.into() - } else if let Some(path) = table.get("path") { - let path = crate_root + } + if let Some(value) = table.get("version") { + src = src.set_version(value.as_str().ok_or_else(|| { + invalid_type(key, "version", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(path) = table.get("path") { + let path = + crate_root .join(path.as_str().ok_or_else(|| { invalid_type(key, "path", path.type_name(), "string") })?); - let mut src = PathSource::new(path); - if let Some(value) = table.get("version") { - src = src.set_version(value.as_str().ok_or_else(|| { - invalid_type(key, "version", value.type_name(), "string") - })?); - } - src.into() - } else if let Some(version) = table.get("version") { - let src = RegistrySource::new(version.as_str().ok_or_else(|| { + let mut src = PathSource::new(path); + if let Some(value) = table.get("version") { + src = src.set_version(value.as_str().ok_or_else(|| { + invalid_type(key, "version", value.type_name(), "string") + })?); + } + src.into() + } else if let Some(version) = table.get("version") { + let src = + RegistrySource::new(version.as_str().ok_or_else(|| { invalid_type(key, "version", version.type_name(), "string") })?); - src.into() - } else if let Some(workspace) = table.get("workspace") { - let workspace_bool = workspace.as_bool().ok_or_else(|| { - invalid_type(key, "workspace", workspace.type_name(), "bool") - })?; - if !workspace_bool { - anyhow::bail!("`{key}.workspace = false` is unsupported") - } - let src = WorkspaceSource::new(); - src.into() - } else { - let mut msg = format!("Unrecognized dependency source for `{key}`"); - if table.is_empty() { - msg.push_str( - ", expected a table with a `version`, `git`, `path`, or `workspace` key", + src.into() + } else if let Some(workspace) = table.get("workspace") { + let workspace_bool = workspace + .as_bool() + .ok_or_else(|| invalid_type(key, "workspace", workspace.type_name(), "bool"))?; + if !workspace_bool { + anyhow::bail!("`{key}.workspace = false` is unsupported") + } + let src = WorkspaceSource::new(); + src.into() + } else { + let mut msg = format!("unrecognized dependency source for `{key}`"); + if table.is_empty() { + msg.push_str( + ", expected a local path, Git repository, version, or workspace dependency to be specified", ); - } - anyhow::bail!(msg); - }; + } + anyhow::bail!(msg); + }; let registry = if let Some(value) = table.get("registry") { Some( value diff --git a/tests/testsuite/cargo_add/empty_dep_table/stderr.log b/tests/testsuite/cargo_add/empty_dep_table/stderr.log index 00b1db5e369..3acd7b944ec 100644 --- a/tests/testsuite/cargo_add/empty_dep_table/stderr.log +++ b/tests/testsuite/cargo_add/empty_dep_table/stderr.log @@ -1 +1 @@ -error: Unrecognized dependency source for `your-face`, expected a table with a `version`, `git`, `path`, or `workspace` key +error: unrecognized dependency source for `your-face`, expected a local path, Git repository, version, or workspace dependency to be specified