From 2c7f770aa9b094afca4454c8e85c8a963112ec32 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Sat, 23 Apr 2022 03:10:34 -0500 Subject: [PATCH] Only merge top-level array when merging `languages.toml` (#2215) * Revert "Revert "override nested arrays when merging TOML (#2145)"" This reverts commit 35d2693630a4ec29a654704bc4be47badb8d6070. * flip top-level table merging flag --- helix-loader/src/config.rs | 2 +- helix-loader/src/lib.rs | 101 ++++++++++++++++++++++++++++--------- 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/helix-loader/src/config.rs b/helix-loader/src/config.rs index 5dc2d6b67..a8c843612 100644 --- a/helix-loader/src/config.rs +++ b/helix-loader/src/config.rs @@ -19,7 +19,7 @@ pub fn user_lang_config() -> Result { .into_iter() .chain([default_lang_config()].into_iter()) .fold(toml::Value::Table(toml::value::Table::default()), |a, b| { - crate::merge_toml_values(b, a) + crate::merge_toml_values(b, a, true) }); Ok(config) diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index de2951f8e..767bff7a4 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -98,8 +98,24 @@ pub fn find_root_impl(root: Option<&str>, root_markers: &[String]) -> Vec toml::Value { +/// Merge two TOML documents, merging values from `right` onto `left` +/// +/// When an array exists in both `left` and `right`, `right`'s array is +/// used. When a table exists in both `left` and `right`, the merged table +/// consists of all keys in `left`'s table unioned with all keys in `right` +/// with the values of `right` being merged recursively onto values of +/// `left`. +/// +/// `merge_toplevel_arrays` controls whether a top-level array in the TOML +/// document is merged instead of overridden. This is useful for TOML +/// documents that use a top-level array of values like the `languages.toml`, +/// where one usually wants to override or add to the array instead of +/// replacing it altogether. +pub fn merge_toml_values( + left: toml::Value, + right: toml::Value, + merge_toplevel_arrays: bool, +) -> toml::Value { use toml::Value; fn get_name(v: &Value) -> Option<&str> { @@ -108,24 +124,35 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value) -> toml::Value { match (left, right) { (Value::Array(mut left_items), Value::Array(right_items)) => { - left_items.reserve(right_items.len()); - for rvalue in right_items { - let lvalue = get_name(&rvalue) - .and_then(|rname| left_items.iter().position(|v| get_name(v) == Some(rname))) - .map(|lpos| left_items.remove(lpos)); - let mvalue = match lvalue { - Some(lvalue) => merge_toml_values(lvalue, rvalue), - None => rvalue, - }; - left_items.push(mvalue); + // The top-level arrays should be merged but nested arrays should + // act as overrides. For the `languages.toml` config, this means + // that you can specify a sub-set of languages in an overriding + // `languages.toml` but that nested arrays like Language Server + // arguments are replaced instead of merged. + if merge_toplevel_arrays { + left_items.reserve(right_items.len()); + for rvalue in right_items { + let lvalue = get_name(&rvalue) + .and_then(|rname| { + left_items.iter().position(|v| get_name(v) == Some(rname)) + }) + .map(|lpos| left_items.remove(lpos)); + let mvalue = match lvalue { + Some(lvalue) => merge_toml_values(lvalue, rvalue, false), + None => rvalue, + }; + left_items.push(mvalue); + } + Value::Array(left_items) + } else { + Value::Array(right_items) } - Value::Array(left_items) } (Value::Table(mut left_map), Value::Table(right_map)) => { for (rname, rvalue) in right_map { match left_map.remove(&rname) { Some(lvalue) => { - let merged_value = merge_toml_values(lvalue, rvalue); + let merged_value = merge_toml_values(lvalue, rvalue, merge_toplevel_arrays); left_map.insert(rname, merged_value); } None => { @@ -143,23 +170,22 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value) -> toml::Value { #[cfg(test)] mod merge_toml_tests { use super::merge_toml_values; + use toml::Value; #[test] - fn language_tomls() { - use toml::Value; - - const USER: &str = " + fn language_toml_map_merges() { + const USER: &str = r#" [[language]] - name = \"nix\" - test = \"bbb\" - indent = { tab-width = 4, unit = \" \", test = \"aaa\" } - "; + name = "nix" + test = "bbb" + indent = { tab-width = 4, unit = " ", test = "aaa" } + "#; let base: Value = toml::from_slice(include_bytes!("../../languages.toml")) .expect("Couldn't parse built-in languages config"); let user: Value = toml::from_str(USER).unwrap(); - let merged = merge_toml_values(base, user); + let merged = merge_toml_values(base, user, true); let languages = merged.get("language").unwrap().as_array().unwrap(); let nix = languages .iter() @@ -179,4 +205,33 @@ mod merge_toml_tests { // We didn't change comment-token so it should be same assert_eq!(nix.get("comment-token").unwrap().as_str().unwrap(), "#"); } + + #[test] + fn language_toml_nested_array_merges() { + const USER: &str = r#" + [[language]] + name = "typescript" + language-server = { command = "deno", args = ["lsp"] } + "#; + + let base: Value = toml::from_slice(include_bytes!("../../languages.toml")) + .expect("Couldn't parse built-in languages config"); + let user: Value = toml::from_str(USER).unwrap(); + + let merged = merge_toml_values(base, user, true); + let languages = merged.get("language").unwrap().as_array().unwrap(); + let ts = languages + .iter() + .find(|v| v.get("name").unwrap().as_str().unwrap() == "typescript") + .unwrap(); + assert_eq!( + ts.get("language-server") + .unwrap() + .get("args") + .unwrap() + .as_array() + .unwrap(), + &vec![Value::String("lsp".into())] + ) + } }