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.

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

Jump to bottom

Override nested arrays when merging TOML #2145

Merged
merged 1 commit into from
Apr 20, 2022
Merged

Override nested arrays when merging TOML #2145

merged 1 commit into from
Apr 20, 2022

Conversation

the-mikedavis
Copy link
Member

We merge the elements of arrays for the top-level array. For
languages.toml, this is the array of languages. For any nested
arrays, we simply take the right array as-is instead of using
the union of left and right.

closes #1000

@the-mikedavis
Copy link
Member Author

I'm pretty sure this doesn't affect the merging behavior for config.toml but maybe we should use a separate function for the languages.toml just to be sure?

@sudormrfbin
Copy link
Member

I'm pretty sure this doesn't affect the merging behavior for config.toml but maybe we should use a separate function for the languages.toml just to be sure?

editor.shell for example is an array which defaults to ["sh", "-c"]. Merging this with the user config value would be the wrong thing here, so I think merging should only be done for languages.toml.

@the-mikedavis
override nested arrays when merging TOML
5c9e5e5 
We merge the elements of arrays for the top-level array. For
`languages.toml`, this is the array of languages. For any nested
arrays, we simply take the `right` array as-is instead of using
the union of `left` and `right`.

closes helix-editor#1000
the-mikedavis
the-mikedavis commented Apr 18, 2022
View reviewed changes
helix-loader/src/config.rs
crate::merge_toml_values(b, a)
crate::merge_toml_values(b, a, false)
Copy link
Member Author

@the-mikedavis the-mikedavis Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Before #2129 we weren't merging config.toml anywhere. Now that #2129 has been merged we can disable the array merging for config.toml by passing in that flag as false here.

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 you linked the wrong PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! 🤦

I meant #1249

@archseer archseer merged commit c8cfd0b into helix-editor:master Apr 20, 2022
@the-mikedavis the-mikedavis deleted the md-toml-merge-array-overrides branch April 20, 2022 01:48
@poliorcetics poliorcetics mentioned this pull request Apr 20, 2022
Closed
archseer added a commit that referenced this pull request Apr 20, 2022
@archseer
Revert "override nested arrays when merging TOML (#2145)"
35d2693 
Looks like there's some follow-up issues

This reverts commit c8cfd0b.
@the-mikedavis the-mikedavis mentioned this pull request Apr 21, 2022
Closed
the-mikedavis added a commit to the-mikedavis/helix that referenced this pull request Apr 21, 2022
@the-mikedavis
Revert "Revert "override nested arrays when merging TOML (helix-edito…
2df735f 
…r#2145)""

This reverts commit 35d2693.
@the-mikedavis the-mikedavis mentioned this pull request Apr 21, 2022
Merged
@Philipp-M Philipp-M mentioned this pull request Apr 22, 2022
Closed
1 task
archseer pushed a commit that referenced this pull request Apr 23, 2022
@the-mikedavis
Only merge top-level array when merging languages.toml (#2215)
2c7f770 
* Revert "Revert "override nested arrays when merging TOML (#2145)""

This reverts commit 35d2693.

* flip top-level table merging flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirawi kirawi kirawi left review comments

@archseer archseer archseer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrectly merging built-in and user config languages.toml files for typescript lsp
4 participants
@the-mikedavis @sudormrfbin @archseer @kirawi