Skip to content

Refactor Config::load #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thomie wants to merge 2 commits into
base: fix-merge-toml-values
Choose a base branch
from
Open

Refactor Config::load #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdc2303
Fix merge bug in Config::load
thomie Apr 11, 2025
c29a9f7
Refactor Config::load
thomie Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 76 additions & 10 deletions helix-loader/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,39 +152,102 @@ pub fn default_log_file() -> PathBuf {
cache_dir().join("helix.log")
}

pub struct MergeStrategy {
pub array: MergeMode,
pub table: MergeMode,
}

pub enum MergeMode {
Never,
Always,
MaxDepth(usize),
}

impl MergeMode {
pub fn should_merge(&self, depth: usize) -> bool {
match self {
MergeMode::Always => true,
MergeMode::MaxDepth(max_depth) => depth < *max_depth,
MergeMode::Never => false,
}
}
}

/// Merge two TOML documents, merging values from `right` onto `left`
///
/// `merge_depth` sets the nesting depth up to which values are merged instead
/// of overridden.
/// `max_merge_depth` sets the nesting depth up to which values are merged
/// instead of overridden.
///
/// When an array exists in both `left` and `right`, the merged array is formed
/// by concatenating `left`'s elements with `right`'s. But if any elements share
/// the same `name` field, they are merged recursively and included only once.
///
/// 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`.
///
/// Setting `max_merge_depth` is useful for TOML documents that use a
/// top-level array of values, where the top-level arrays should be merged
/// but nested arrays should act as overrides. For the `languages.toml`
/// config for example, 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.
///
/// `crate::merge_toml_values(a, b, 3)` combines, for example:
///
/// b:
/// a:
/// ```toml
/// [[language]]
/// name = "toml"
/// scope = "source.toml"
/// language-server = { command = "taplo", args = ["lsp", "stdio"] }
/// ```
/// a:
/// b:
/// ```toml
/// [[language]]
/// name = "toml"
/// language-server = { command = "/usr/bin/taplo" }
/// ```
///
/// into:
/// ```toml
/// [[language]]
/// name = "toml"
/// scope = "source.toml"
/// language-server = { command = "/usr/bin/taplo" }
/// ```
///
/// thus it overrides the third depth-level of b with values of a if they exist,
/// thus it overrides the third depth-level of a with values of b if they exist,
/// but otherwise merges their values
pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usize) -> toml::Value {
pub fn merge_toml_values(
left: toml::Value,
right: toml::Value,
max_merge_depth: usize,
) -> toml::Value {
merge_toml_values_with_strategy(
left,
right,
&MergeStrategy {
array: MergeMode::MaxDepth(max_merge_depth),
table: MergeMode::MaxDepth(max_merge_depth),
},
)
}

pub fn merge_toml_values_with_strategy(
left: toml::Value,
right: toml::Value,
strategy: &MergeStrategy,
) -> toml::Value {
merge_toml_values_recursive(left, right, strategy, 0)
}

fn merge_toml_values_recursive(
left: toml::Value,
right: toml::Value,
strategy: &MergeStrategy,
depth: usize,
) -> toml::Value {
use toml::Value;

fn get_name(v: &Value) -> Option<&str> {
Expand All @@ -193,7 +256,7 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi

match (left, right) {
(Value::Array(mut left_items), Value::Array(right_items)) => {
if merge_depth > 0 {
if strategy.array.should_merge(depth) {
left_items.reserve(right_items.len());
for rvalue in right_items {
let lvalue = get_name(&rvalue)
Expand All @@ -202,7 +265,9 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi
})
.map(|lpos| left_items.remove(lpos));
let mvalue = match lvalue {
Some(lvalue) => merge_toml_values(lvalue, rvalue, merge_depth - 1),
Some(lvalue) => {
merge_toml_values_recursive(lvalue, rvalue, strategy, depth + 1)
}
None => rvalue,
};
left_items.push(mvalue);
Expand All @@ -213,11 +278,12 @@ pub fn merge_toml_values(left: toml::Value, right: toml::Value, merge_depth: usi
}
}
(Value::Table(mut left_map), Value::Table(right_map)) => {
if merge_depth > 0 {
if strategy.table.should_merge(depth) {
for (rname, rvalue) in right_map {
match left_map.remove(&rname) {
Some(lvalue) => {
let merged_value = merge_toml_values(lvalue, rvalue, merge_depth - 1);
let merged_value =
merge_toml_values_recursive(lvalue, rvalue, strategy, depth + 1);
left_map.insert(rname, merged_value);
}
None => {
Expand Down
156 changes: 90 additions & 66 deletions helix-term/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use crate::keymap;
use crate::keymap::{merge_keys, KeyTrie};
use helix_loader::merge_toml_values;
use helix_loader::{merge_toml_values_with_strategy, MergeMode, MergeStrategy};
use helix_view::document::Mode;
use serde::Deserialize;
use std::collections::HashMap;
use std::fmt::Display;
use std::fs;
use std::io::Error as IOError;
use std::path::PathBuf;
use toml::de::Error as TomlError;

#[derive(Debug, Clone, PartialEq)]
Expand All @@ -24,6 +25,19 @@ pub struct ConfigRaw {
pub editor: Option<toml::Value>,
}

impl ConfigRaw {
pub fn load(path: PathBuf) -> Result<Option<ConfigRaw>, ConfigLoadError> {
match fs::read_to_string(path) {
// Don't treat a missing config file as an error.
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None),
Err(e) => Err(ConfigLoadError::Error(e)),
Ok(s) => toml::from_str(&s)
.map(Some)
.map_err(ConfigLoadError::BadConfig),
}
}
}

impl Default for Config {
fn default() -> Config {
Config {
Expand Down Expand Up @@ -56,73 +70,40 @@ impl Display for ConfigLoadError {
}

impl Config {
pub fn load(
global: Result<String, ConfigLoadError>,
local: Result<String, ConfigLoadError>,
) -> Result<Config, ConfigLoadError> {
let global_config: Result<ConfigRaw, ConfigLoadError> =
global.and_then(|file| toml::from_str(&file).map_err(ConfigLoadError::BadConfig));
let local_config: Result<ConfigRaw, ConfigLoadError> =
local.and_then(|file| toml::from_str(&file).map_err(ConfigLoadError::BadConfig));
let res = match (global_config, local_config) {
(Ok(global), Ok(local)) => {
let mut keys = keymap::default();
if let Some(global_keys) = global.keys {
merge_keys(&mut keys, global_keys)
}
if let Some(local_keys) = local.keys {
merge_keys(&mut keys, local_keys)
}

let editor = match (global.editor, local.editor) {
(None, None) => helix_view::editor::Config::default(),
(None, Some(val)) | (Some(val), None) => {
val.try_into().map_err(ConfigLoadError::BadConfig)?
}
(Some(global), Some(local)) => merge_toml_values(global, local, 3)
.try_into()
.map_err(ConfigLoadError::BadConfig)?,
};

Config {
theme: local.theme.or(global.theme),
keys,
editor,
}
/// Merge a ConfigRaw value into a Config.
pub fn apply(&mut self, opt_config_raw: Option<ConfigRaw>) -> Result<(), ConfigLoadError> {
if let Some(config_raw) = opt_config_raw {
if let Some(theme) = config_raw.theme {
self.theme = Some(theme)
}
// if any configs are invalid return that first
(_, Err(ConfigLoadError::BadConfig(err)))
| (Err(ConfigLoadError::BadConfig(err)), _) => {
return Err(ConfigLoadError::BadConfig(err))
if let Some(keymap) = config_raw.keys {
merge_keys(&mut self.keys, keymap)
}
(Ok(config), Err(_)) | (Err(_), Ok(config)) => {
let mut keys = keymap::default();
if let Some(keymap) = config.keys {
merge_keys(&mut keys, keymap);
}
Config {
theme: config.theme,
keys,
editor: config.editor.map_or_else(
|| Ok(helix_view::editor::Config::default()),
|val| val.try_into().map_err(ConfigLoadError::BadConfig),
)?,
}
if let Some(editor) = config_raw.editor {
// We only know how to merge toml values, so convert back to toml first.
let val = toml::Value::try_from(&self.editor).unwrap();
self.editor = merge_toml_values_with_strategy(
val,
editor,
&MergeStrategy {
array: MergeMode::Never,
table: MergeMode::Always,
},
)
.try_into()
.map_err(ConfigLoadError::BadConfig)?
}

// these are just two io errors return the one for the global config
(Err(err), Err(_)) => return Err(err),
};

Ok(res)
}
Ok(())
}

pub fn load_default() -> Result<Config, ConfigLoadError> {
let global_config =
fs::read_to_string(helix_loader::config_file()).map_err(ConfigLoadError::Error);
let local_config = fs::read_to_string(helix_loader::workspace_config_file())
.map_err(ConfigLoadError::Error);
Config::load(global_config, local_config)
let mut config = Config::default();
let global = ConfigRaw::load(helix_loader::config_file())?;
let local = ConfigRaw::load(helix_loader::workspace_config_file())?;
config.apply(global)?;
config.apply(local)?;
Ok(config)
}
}

Expand All @@ -131,11 +112,54 @@ mod tests {
use super::*;

impl Config {
fn load_test(config: &str) -> Config {
Config::load(Ok(config.to_owned()), Err(ConfigLoadError::default())).unwrap()
fn load_test(global: &str, local: &str) -> Config {
let mut config = Config::default();
let global = Some(toml::from_str(&global).unwrap());
let local = Some(toml::from_str(&local).unwrap());
config.apply(global).unwrap();
config.apply(local).unwrap();
config
}
}

#[test]
fn should_merge_editor_config_tables() {
let global = r#"
[editor.statusline]
mode.insert = "INSERT"
mode.select = "SELECT"
"#;
let local = r#"
[editor.statusline]
mode.select = "VIS"
"#;
let config = Config::load_test(global, local);
assert_eq!(config.editor.statusline.mode.normal, "NOR"); // Default
assert_eq!(config.editor.statusline.mode.insert, "INSERT"); // Global
assert_eq!(config.editor.statusline.mode.select, "VIS"); // Local
}

#[test]
fn should_override_editor_config_arrays() {
let global = r#"
[editor]
shell = ["bash", "-c"]
"#;
let local = r#"
[editor]
shell = ["fish", "-c"]
"#;
let config = Config::load_test(global, local);
assert_eq!(config.editor.shell, ["fish", "-c"]);
}

#[test]
fn load_non_existing_config() {
let path = PathBuf::from(r"does-not-exist");
let result = ConfigRaw::load(path);
assert!(result.is_ok_and(|x| x.is_none()));
}

#[test]
fn parsing_keymaps_config_file() {
use crate::keymap;
Expand Down Expand Up @@ -166,7 +190,7 @@ mod tests {
);

assert_eq!(
Config::load_test(sample_keymaps),
Config::load_test(sample_keymaps, ""),
Config {
keys,
..Default::default()
Expand All @@ -177,7 +201,7 @@ mod tests {
#[test]
fn keys_resolve_to_correct_defaults() {
// From serde default
let default_keys = Config::load_test("").keys;
let default_keys = Config::load_test("", "").keys;
assert_eq!(default_keys, keymap::default());

// From the Default trait
Expand Down