Skip to content

Merge variable tables instead of bailing #64

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
Closed
wants to merge 1 commit into from
Closed

Merge variable tables instead of bailing #64

wants to merge 1 commit into from

Conversation

runiq
Copy link

@runiq runiq commented May 19, 2021

This allows to have nested tables for variables.

I'm using it in my Neovim config to do some package-specific configuration if (and only if) a package is enabled:

# global.toml
[rustdev.variables]
neovim.subpackages.rustdev = true
[luadev.variables]
neovim.subpackages.luadev = true

With this, I can loop over the neovim.subpackages variable and do something for all enabled packages only—like run some package-specific code:

-- subpackages.lua
local subpackages = {
  {{#each neovim.subpackages}}
  '{{@key}}',
  {{/each}}
}
for _, subpackage in ipairs(subpackages) do
  require(subpackage)

Now, if I decide to disable the luadev package, I won't have to change subpackages.lua in any way—since the variable neovim.subpackages.luadev doesn't exist anymore, it won't be in the subpackages list.

I'm pretty sure this would enable other functionality as well, considering the power of the handlebars templating facility.

@runiq
Copy link
Author

runiq commented May 19, 2021

So I figured out how to do what I want to do without this PR. If you want it anyways, I could still keep it open, though? Your call. :)

@SuperCuber
Copy link
Owner

Hey, the CI went through some changes to make sure it works for older versions of Linux. Please merge the changes from master to your branch and re-run the CI (I might need to re-approve it)

@runiq
Merge variable tables instead of bailing
1712b25 
This allows to have nested tables for variables.
@runiq
Copy link
Author

runiq commented May 21, 2021 •
edited
Loading

Done! (Edit: And yeah, looks like you need to re-approve)

SuperCuber
SuperCuber reviewed May 30, 2021
View reviewed changes
src/config.rs
@@ -199,6 +199,19 @@ fn recursive_extend_map(
}
}

/// Merge two TOML tables
Copy link
Owner

Choose a reason for hiding this comment

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

This function already exists:

dotter/src/config.rs

Line 179 in 1712b25

fn recursive_extend_map(

You should either rewrite the below code to use it or rewrite the code that uses that function to use this instead

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow, I'm sorry, how did I not notice that? Will do, thank you!

@LucasOe
Copy link

LucasOe commented Jun 25, 2022

I created a fix for this issue. Is there any way I can add to this pull request, or do I have to create a new one?

@SuperCuber
Copy link
Owner

I created a fix for this issue. Is there any way I can add to this pull request, or do I have to create a new one?

I think you need to create a new one. For bonus points, add a test that fails without the fix and passes with the fix :)

@LucasOe LucasOe mentioned this pull request Jun 26, 2022
Closed
@SuperCuber
Copy link
Owner

Closed, see #102

@SuperCuber SuperCuber closed this Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperCuber SuperCuber SuperCuber left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@runiq @SuperCuber @LucasOe