Fix default evil editor config #74
Conversation
merge_toplevel_arrays is no longer a parameter for this method, and the example fits better in a doc comment.
Currently `Config::load` merges editor configs with
`merge_toml_values(global, local, 3)`
Besides the puzzling use of the magic number `3`, this is a bug,
because it concatenates arrays from the global config file with
those from the local config file.
For example `editor.shell=["bash", "-c"]` and
`editor.shell=["fish", "-c"]` would become
`editor.shell=["bash", "-c", "fish", "-c"]`.
This commit adds a new function `merge_toml_values_with_strategy` and
changes `Config::load` to call:
```
merge_toml_values_with_strategy(
global,
local,
&MergeStrategy {
array: MergeMode::Never,
table: MergeMode::Always,
},
)
```
For backward compatability, the original function `merge_toml_values`
calls:
```
merge_toml_values_with_strategy(
left,
right,
&MergeStrategy {
array: MergeMode::MaxDepth(max_merge_depth),
table: MergeMode::MaxDepth(max_merge_depth),
},
)
```
, so there should be no change in the way themes and languages.toml
are merged.
Also update doc string and add tests.
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 #13332.
thomie
changed the title
|
This looks great, and I hope we'll get some early feedback from upstream soon in helix-editor/helix#13332, such that I can anticipate the "risk" of merging this. Either way, I'd love to get this in the next release (which I might prepare this weekend), so I'll try to decide soon enough. |
|
@thomie I think I'll take a leap of faith on this one; I'd like to merge this in the coming days without waiting much longer for upstream feedback. But in its current form, this PR is very "invasive" in the upstream code. Do you have an idea on how we could stay a bit safer here? Maybe we could treat the bugfixing part as an evil feature (that would mean leaving upstream code as untouched as possible), as long as it's not handled upstream? In that case, I'd imagine we could achieve hooking patterns as follows: pub fn load(
global: Result<String, ConfigLoadError>,
local: Result<String, ConfigLoadError>,
) -> Result<Config, ConfigLoadError> {
if evil {
return evil_load(...);
}
// ...What do you think? |
This may look like a lot of changes, but everything except the last commit (which is tiny) should go into helix upstream.
With theses change, all the
default_evileditor settings apply even when the config file contains an editor section (#70).And
statusline.modecan be overridden from the config file (#53) (fixed by removing the "HACK" from config.rs).Closes #53
(once #70 is merged as well)and #73.