diff --git a/cli/src/config.rs b/cli/src/config.rs index af6b8f5e0..aba8b2793 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -469,36 +469,14 @@ pub fn remove_config_value_from_file( path: &Path, ) -> Result<(), CommandError> { // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut doc = load_config_file_or_empty(ConfigSource::User, path)?.data; - - // Find target table - let mut key_iter = key.components(); - let last_key = key_iter.next_back().expect("key must not be empty"); - let target_table = key_iter.try_fold(doc.as_table_mut(), |table, key| { - table - .get_mut(key) - .ok_or_else(|| user_error(format!(r#""{key}" doesn't exist"#))) - .and_then(|table| { - table - .as_table_mut() - .ok_or_else(|| user_error(format!(r#""{key}" is not a table"#))) - }) - })?; - - // Remove config value - match target_table.entry(last_key) { - toml_edit::Entry::Occupied(entry) => { - if entry.get().is_table() { - return Err(user_error(format!("Won't remove table {key}"))); - } - entry.remove(); - } - toml_edit::Entry::Vacant(_) => { - return Err(user_error(format!(r#""{key}" doesn't exist"#))); - } + let mut layer = load_config_file_or_empty(ConfigSource::User, path)?; + let old_value = layer + .delete_value(key) + .map_err(|err| user_error_with_message(format!("Failed to unset {key}"), err))?; + if old_value.is_none() { + return Err(user_error(format!(r#""{key}" doesn't exist"#))); } - - write_config(path, &doc) + write_config(path, &layer.data) } /// Command name and arguments specified by config. diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 191a2507b..d974463c9 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -655,7 +655,7 @@ fn test_config_unset_inline_table_key() { &["config", "unset", "--user", "inline-table.foo"], ); - insta::assert_snapshot!(stderr, @r#"Error: "inline-table" is not a table"#); + insta::assert_snapshot!(stderr, @r#"Error: "inline-table.foo" doesn't exist"#); } #[test] @@ -685,7 +685,10 @@ fn test_config_unset_table_like() { test_env.env_root(), &["config", "unset", "--user", "non-inline-table"], ); - insta::assert_snapshot!(stderr, @"Error: Won't remove table non-inline-table"); + insta::assert_snapshot!(stderr, @r" + Error: Failed to unset non-inline-table + Caused by: Would delete entire table non-inline-table + "); let user_config_toml = std::fs::read_to_string(&user_config_path).unwrap(); insta::assert_snapshot!(user_config_toml, @r" diff --git a/lib/src/config.rs b/lib/src/config.rs index 7557feba6..c1beb4fdc 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -95,6 +95,12 @@ pub enum ConfigUpdateError { /// Dotted config name path. name: String, }, + /// Table exists at the path, which shouldn't be deleted. + #[error("Would delete entire table {name}")] + WouldDeleteTable { + /// Dotted config name path. + name: String, + }, } /// Extension methods for `Result`. @@ -392,6 +398,39 @@ impl ConfigLayer { } } } + + /// Deletes value specified by the `name` path. Returns old value if any. + /// + /// Returns `Ok(None)` if middle node wasn't a table or a value wasn't + /// found. Returns `Err` if attempted to delete a table. + pub fn delete_value( + &mut self, + name: impl ToConfigNamePath, + ) -> Result, ConfigUpdateError> { + let would_delete_table = |name| ConfigUpdateError::WouldDeleteTable { name }; + let name = name.into_name_path(); + let name = name.borrow(); + let mut keys = name.components(); + let leaf_key = keys + .next_back() + .ok_or_else(|| would_delete_table(name.to_string()))?; + let root_table = self.data.as_table_mut(); + let Some(parent_table) = + keys.try_fold(root_table, |table, key| table.get_mut(key)?.as_table_mut()) + else { + return Ok(None); + }; + match parent_table.entry(leaf_key) { + toml_edit::Entry::Occupied(entry) => { + if !entry.get().is_value() { + return Err(would_delete_table(name.to_string())); + } + let old_item = entry.remove(); + Ok(Some(old_item.into_value().unwrap())) + } + toml_edit::Entry::Vacant(_) => Ok(None), + } + } } /// Looks up item from the `root_item`. Returns `Some(item)` if an item found at @@ -759,6 +798,53 @@ mod tests { "#); } + #[test] + fn test_config_layer_delete_value() { + let mut layer = ConfigLayer::empty(ConfigSource::User); + // Cannot delete the root table + assert_matches!( + layer.delete_value(ConfigNamePathBuf::root()), + Err(ConfigUpdateError::WouldDeleteTable { name }) if name.is_empty() + ); + + // Insert some values + layer.set_value("foo", 1).unwrap(); + layer.set_value("bar.baz.blah", "2").unwrap(); + layer + .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")])) + .unwrap(); + insta::assert_snapshot!(layer.data, @r#" + foo = 1 + + [bar] + qux = { inline = "table" } + + [bar.baz] + blah = "2" + "#); + + // Can delete value + let old_value = layer.delete_value("foo").unwrap(); + assert_eq!(old_value.and_then(|v| v.as_integer()), Some(1)); + // Can delete inline table + let old_value = layer.delete_value("bar.qux").unwrap(); + assert!(old_value.is_some_and(|v| v.is_inline_table())); + // Cannot delete table + assert_matches!( + layer.delete_value("bar"), + Err(ConfigUpdateError::WouldDeleteTable { name }) if name == "bar" + ); + // Deleting a non-table child isn't an error because the value doesn't + // exist + assert_matches!(layer.delete_value("bar.baz.blah.blah"), Ok(None)); + insta::assert_snapshot!(layer.data, @r#" + [bar] + + [bar.baz] + blah = "2" + "#); + } + #[test] fn test_stacked_config_layer_order() { let empty_data = || DocumentMut::new();