Skip to content
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.

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

Refactor 'helix-loader::merge_toml_values' to use a 'merge-depth' instead of 'merge_toplevel_arrays' #3080

Merged

Conversation

Philipp-M
Copy link
Contributor

This ensures that other values than just the top-level arrays are overridden, and makes this function generally more flexible, like merging nested objects, where it makes sense.

For the languages.toml, merge_depth is set to 3 so that top-level language features are merged (like 'scope'), but everything deeper is overridden with the user-config. With the current configuration schema the result should be equivalent to the current behavior.

This change is a requirement for #2507 since language-servers there, are configured via a table instead of an array.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍

The boolean did seem a little hacky, a merge depth looks much more appropriate.

Comment on lines 22 to 38
crate::merge_toml_values(b, a, true)
crate::merge_toml_values(b, a, 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth a comment here explaining why it's 3 for the languages.toml (just here is probably fine, I don't think it needs to be repeated in the tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, great writeup! 👍

…tead of 'merge_toplevel_arrays'

- This ensures that other values than just the arrays are overridden, like nested objects, where it makes sense
- merge_depth is set to 3 so that top-level language features are merged (like 'scope'), but everything deeper is overridden with the user-config
@Philipp-M Philipp-M force-pushed the merge-toml-values-with-merge-depth branch from 400533e to 3828db2 Compare July 22, 2022 09:01
@archseer archseer merged commit 235237d into helix-editor:master Jul 26, 2022
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
…tead of 'merge_toplevel_arrays' (helix-editor#3080)

- This ensures that other values than just the arrays are overridden, like nested objects, where it makes sense
- merge_depth is set to 3 so that top-level language features are merged (like 'scope'), but everything deeper is overridden with the user-config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants