Skip to content

Commit 3aa5a24

Browse files
thomiethomie
committed
Refactor Config::load
Split `Config::load` into 2 smaller functions: 1. A function to load a `ConfigRaw` from a path 2. A function to apply a `ConfigRaw` to a `Config` The resulting code is both shorter, more readable, and more extensible (e.g., it can support more than two config files). This refactor revealed the bug described in helix-editor#13332.
1 parent bdc2303 commit 3aa5a24

File tree

1 file changed

+55
-69
lines changed

1 file changed

+55
-69
lines changed

‎helix-term/src/config.rs

+55-69
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use std::collections::HashMap;
77
use std::fmt::Display;
88
use std::fs;
99
use std::io::Error as IOError;
10+
use std::path::PathBuf;
1011
use toml::de::Error as TomlError;
1112

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

28+
impl ConfigRaw {
29+
pub fn load(path: PathBuf) -> Result<Option<ConfigRaw>, ConfigLoadError> {
30+
match fs::read_to_string(path) {
31+
// Don't treat a missing config file as an error.
32+
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None),
33+
Err(e) => Err(ConfigLoadError::Error(e)),
34+
Ok(s) => toml::from_str(&s)
35+
.map(Some)
36+
.map_err(ConfigLoadError::BadConfig),
37+
}
38+
}
39+
}
40+
2741
impl Default for Config {
2842
fn default() -> Config {
2943
Config {
@@ -56,80 +70,40 @@ impl Display for ConfigLoadError {
5670
}
5771

5872
impl Config {
59-
pub fn load(
60-
global: Result<String, ConfigLoadError>,
61-
local: Result<String, ConfigLoadError>,
62-
) -> Result<Config, ConfigLoadError> {
63-
let global_config: Result<ConfigRaw, ConfigLoadError> =
64-
global.and_then(|file| toml::from_str(&file).map_err(ConfigLoadError::BadConfig));
65-
let local_config: Result<ConfigRaw, ConfigLoadError> =
66-
local.and_then(|file| toml::from_str(&file).map_err(ConfigLoadError::BadConfig));
67-
let res = match (global_config, local_config) {
68-
(Ok(global), Ok(local)) => {
69-
let mut keys = keymap::default();
70-
if let Some(global_keys) = global.keys {
71-
merge_keys(&mut keys, global_keys)
72-
}
73-
if let Some(local_keys) = local.keys {
74-
merge_keys(&mut keys, local_keys)
75-
}
76-
77-
let editor = match (global.editor, local.editor) {
78-
(None, None) => helix_view::editor::Config::default(),
79-
(None, Some(val)) | (Some(val), None) => {
80-
val.try_into().map_err(ConfigLoadError::BadConfig)?
81-
}
82-
(Some(global), Some(local)) => merge_toml_values_with_strategy(
83-
global,
84-
local,
85-
&MergeStrategy {
86-
array: MergeMode::Never,
87-
table: MergeMode::Always,
88-
},
89-
)
90-
.try_into()
91-
.map_err(ConfigLoadError::BadConfig)?,
92-
};
93-
94-
Config {
95-
theme: local.theme.or(global.theme),
96-
keys,
97-
editor,
98-
}
73+
/// Merge a ConfigRaw value into a Config.
74+
pub fn apply(&mut self, opt_config_raw: Option<ConfigRaw>) -> Result<(), ConfigLoadError> {
75+
if let Some(config_raw) = opt_config_raw {
76+
if let Some(theme) = config_raw.theme {
77+
self.theme = Some(theme)
9978
}
100-
// if any configs are invalid return that first
101-
(_, Err(ConfigLoadError::BadConfig(err)))
102-
| (Err(ConfigLoadError::BadConfig(err)), _) => {
103-
return Err(ConfigLoadError::BadConfig(err))
79+
if let Some(keymap) = config_raw.keys {
80+
merge_keys(&mut self.keys, keymap)
10481
}
105-
(Ok(config), Err(_)) | (Err(_), Ok(config)) => {
106-
let mut keys = keymap::default();
107-
if let Some(keymap) = config.keys {
108-
merge_keys(&mut keys, keymap);
109-
}
110-
Config {
111-
theme: config.theme,
112-
keys,
113-
editor: config.editor.map_or_else(
114-
|| Ok(helix_view::editor::Config::default()),
115-
|val| val.try_into().map_err(ConfigLoadError::BadConfig),
116-
)?,
117-
}
82+
if let Some(editor) = config_raw.editor {
83+
// We only know how to merge toml values, so convert back to toml first.
84+
let val = toml::Value::try_from(&self.editor).unwrap();
85+
self.editor = merge_toml_values_with_strategy(
86+
val,
87+
editor,
88+
&MergeStrategy {
89+
array: MergeMode::Never,
90+
table: MergeMode::Always,
91+
},
92+
)
93+
.try_into()
94+
.map_err(ConfigLoadError::BadConfig)?
11895
}
119-
120-
// these are just two io errors return the one for the global config
121-
(Err(err), Err(_)) => return Err(err),
122-
};
123-
124-
Ok(res)
96+
}
97+
Ok(())
12598
}
12699

127100
pub fn load_default() -> Result<Config, ConfigLoadError> {
128-
let global_config =
129-
fs::read_to_string(helix_loader::config_file()).map_err(ConfigLoadError::Error);
130-
let local_config = fs::read_to_string(helix_loader::workspace_config_file())
131-
.map_err(ConfigLoadError::Error);
132-
Config::load(global_config, local_config)
101+
let mut config = Config::default();
102+
let global = ConfigRaw::load(helix_loader::config_file())?;
103+
let local = ConfigRaw::load(helix_loader::workspace_config_file())?;
104+
config.apply(global)?;
105+
config.apply(local)?;
106+
Ok(config)
133107
}
134108
}
135109

@@ -139,7 +113,12 @@ mod tests {
139113

140114
impl Config {
141115
fn load_test(global: &str, local: &str) -> Config {
142-
Config::load(Ok(global.to_owned()), Ok(local.to_owned())).unwrap()
116+
let mut config = Config::default();
117+
let global = toml::from_str(&global).unwrap();
118+
let local = toml::from_str(&local).unwrap();
119+
config.apply(Some(global)).unwrap();
120+
config.apply(Some(local)).unwrap();
121+
config
143122
}
144123
}
145124

@@ -174,6 +153,13 @@ mod tests {
174153
assert_eq!(config.editor.shell, ["fish", "-c"]);
175154
}
176155

156+
#[test]
157+
fn load_non_existing_config() {
158+
let path = PathBuf::from(r"does-not-exist");
159+
let result = ConfigRaw::load(path);
160+
assert!(result.is_ok_and(|x| x.is_none()));
161+
}
162+
177163
#[test]
178164
fn parsing_keymaps_config_file() {
179165
use crate::keymap;

0 commit comments

Comments
 (0)